Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V3 geom2 no sides #1182

Merged
merged 15 commits into from
Jan 8, 2023
Merged

V3 geom2 no sides #1182

merged 15 commits into from
Jan 8, 2023

Conversation

platypii
Copy link
Contributor

@platypii platypii commented Jan 3, 2023

This PR changes geom2 from using sides to using outlines. This will improve performance, reduce memory allocations, and simplify a bunch of operations.

The main benefit is that for operations like booleans, extrusions, and expansion, the first step is to produce outlines from a geom2. To produce outlines from sides requires running a depth first search of the sides. With this PR, there is no need to run a graph traversal, because the data structure is already stored as outlines.

Credit to @briansturgill for the idea in #1120

To make these changes work, I needed to update the following:

  • Change internal data structure of geom2
  • Move geom2.toOutlines to geom2.fromSides
  • Refactor compact binary representation of outlines
  • Update tests to use new geom2.create with outlines
  • Update tests for shuffling of the output points
  • Reduce usage of geom2.toSides
  • Fix a martinez bug that was revealed by the JSCAD test suite [1]

Sorry for the large PR. This will help prepare for future improvements for things like the expansions which I want to look at next. Let’s make the V3 geom2 engine into a ferrari :-D

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

@platypii platypii requested review from z3dev and hrgdavor January 3, 2023 19:13
hrgdavor
hrgdavor previously approved these changes Jan 3, 2023
Copy link
Contributor

@hrgdavor hrgdavor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice effort. I am very much in favor of this.

Unrelated comment. Also for some next PR it will be I think ok to remove compactBinary. during my explorations of worker code, I think it will be just fine to do all the calculations in worker, and also IO for export, so there is no need to transfer models to main thread. TypedArrays that are used to provide webgl data for displaying is transferable and already optimal for postMessage.

@z3dev
Copy link
Member

z3dev commented Jan 4, 2023

@platypii This is so cool. This is like one of the most talked about change for V2 but we opted out in order to focus on fixing the broken API (V1 was a mess). And, I think this would never be possible without the improvements made over the last year to V2. The result of this PR will be huge, and allow the 2D API to become much more useful.

Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@platypii great start but i have a few change requests, and a few questions as well.

packages/modeling/src/geometries/geom2/create.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/create.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/fromPoints.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/fromSides.js Outdated Show resolved Hide resolved
packages/modeling/src/operations/modifiers/snap.js Outdated Show resolved Hide resolved
packages/modeling/src/primitives/polygon.js Outdated Show resolved Hide resolved
@platypii platypii requested a review from z3dev January 6, 2023 20:12
@platypii platypii requested a review from z3dev January 7, 2023 09:20
Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@platypii getting close now. sorry for the nits.

packages/modeling/src/geometries/geom2/create.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/fromSides.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/index.js Outdated Show resolved Hide resolved
packages/modeling/src/geometries/geom2/reverse.js Outdated Show resolved Hide resolved
@platypii platypii requested a review from z3dev January 8, 2023 02:41
Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@platypii super job. thanks again.

i think those little discussions were worth the extra time as well.

@platypii
Copy link
Contributor Author

platypii commented Jan 8, 2023

i think those little discussions were worth the extra time as well.

Definitely. Thanks for the detailed reviews!

@z3dev z3dev merged commit ed80f04 into jscad:V3 Jan 8, 2023
@platypii platypii deleted the V3-no-sides branch January 8, 2023 04:21
@hrgdavor
Copy link
Contributor

hrgdavor commented Jan 8, 2023

@z3dev thanks for the detailed reviews. I am bit lazy to look that much in detail, and I am more messy than @platypii so I rarely find things to improve :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants