Fix Unaligned Sort Triggering #2997
Open
+144
−185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… later would not be added to the triggering system even though they should have been, this was caused by a buggy equals implementation in AlignableNormal. remove AlignableNormal and replace with a Vector3f & int pair in NormalList and NormalPlanes, associated refactors to remove custom hashing strategy usage again,
douira
force-pushed
the
fix-unaligned-sort-triggering
branch
from
a9ad28e to
18cc7ff
Compare
|
Thanks, this seems good overall. And I'm happy that we aren't subclassing Vector3f anymore. |
jellysquid3
approved these changes
Jan 28, 2025
|
I've added two small bug fix commits. The first for fixing an old but obscure bug that only has relevance with quad splitting, but technically did also exist otherwise. The second corrects a mistake I made during the refactor in this PR. The hashmap of unaligned normals was getting corrupted because I was using a shared scratch object as the hashmap key which mutated the hashmap's key, breaking the hashmap in the process. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug with the plane-based triggering system where unaligned trigger planes would be wrongly not be added to the triggering system if the same same normal was already present.
This was caused by a buggy equals implementation in AlignableNormal.
Since unaligned trigger planes are only used by the very rarely employed dynamic topo sorting mode, this is unlikely to have any actual effects until quad splitting #2993, which makes use of unaligned trigger planes more commonly, is added.
I have gotten reports (at least one, maybe two) of crashes within the sort triggering code (months ago), but I don't know whether those were related to this (and I can't find them anymore on discord).
This PR is a cherry pick of commit d3f4953, originally authored as part of #2993. When/if this gets merged, that PR will probably require merge conflict resolution.