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

Geometry_Engine: Stop checking for SelfIntersections when computing normal for curves? #2797

Closed
IsakNaslundBh opened this issue Apr 22, 2022 · 1 comment · Fixed by #2802
Closed
Assignees
Labels
type:feature New capability or enhancement type:question Ask for further details or start conversation

Comments

@IsakNaslundBh
Copy link
Contributor

Description:

While doing some profiling on some of our heavier geometry methods such as the Boolean intersection/difference/union methods for polycurves/polylines it was found that the clear bottleneck in the execution of those methods was coming from the computation of the normal of the curves, and more specifically, the check if the curves where self intersecting.

for this case, this check is fundamentally redundant, and the method itself simply just raises a warning if the curve actually is self intersecting, but is only raising a warning.

Commenting this check out and re-running saw a performance boost of 80% (!!) for the test case I was running, which involved quite a lot of heavy operations, including computation of voronoi diagrams and similar, with time to compute going from 10 to 2 seconds by just this simple change.

Given this, I am tempted to remove this check from the method, alternatively add another method (something like UnsafeNormal or similar) that simply skips this check (and maybe some others) that can be called when the self intersection check simply is not required for the method to perform the way that is needed.

Glad to get any input on this.

@al-fisher @pawelbaran @FraserGreenroyd

@IsakNaslundBh IsakNaslundBh added type:feature New capability or enhancement type:question Ask for further details or start conversation labels Apr 22, 2022
@IsakNaslundBh IsakNaslundBh self-assigned this Apr 22, 2022
@al-fisher
Copy link
Member

Inclined to agree!
In terms of a performance hit for "just" user feedback as a warning - this seems like something we should not be doing. Or at least have the option to not do. Especially due to this being such a low level method that can be called thousands of times in certain algorithms.

Would support removal of the check as is not a functional change (is simply removal of a warning message and the return value of the method has not possibility of being affected) but agree we should raise an issue to address a general solution to having alternative methods with various levels of checks (affecting return value) and warnings (giving user feedback) activated.

IsakNaslundBh added a commit that referenced this issue Apr 28, 2022
Commenting out IsSelfIntersecting check for normal calculation for now until a better solution is added in #2803
IsakNaslundBh added a commit that referenced this issue May 19, 2022
Commenting out IsSelfIntersecting check for normal calculation for now until a better solution is added in #2803
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement type:question Ask for further details or start conversation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants