Fix CompactInPlace failure #43
Merged
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
dmitry-garshin
approved these changes
Jan 31, 2024
mesh/DMesh3.cs
Outdated
| if (!vertices_refcount.is_dense) | ||
| { | ||
| // find first free vertex, and last used vertex | ||
| int iLastV = MaxVertexID - 1, iCurV = 0; |
There was a problem hiding this comment.
меня смущает, что iLast устанавливаем как MaxVertexID, но iCurV сравниваем с vertices_refcount.max_index
There was a problem hiding this comment.
Тебя зря это смущает, MaxVertexID это
public int MaxVertexID {
get { return vertices_refcount.max_index; }
}Но ради прозрачности я сменю на vertices_refcount.max_index явно
mesh/DMesh3.cs
Outdated
|
|
||
| // move cur forward one, last back one, and then search for next valid | ||
| iLastV--; iCurV++; | ||
| while (vertices_refcount.isValidUnsafe(iLastV) == false) |
There was a problem hiding this comment.
может сюда добавить проверку на iLast >= 0 ?
There was a problem hiding this comment.
У меня были эти мысли, но тут непонятно что лучше. С одной стороны, такая проверка почти во всех случаях избыточна, а актуальна только в некоторых кейсах, которые выглядят как что-то нереалистичное
Добавлю их, на всякий случай
DmitryBubelnikSoftSmile
approved these changes
Feb 1, 2024
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.
Problem
It turned out that
DMesh3.CompactInPlaceworks correctly only ifvertices_refcount,edges_refcountortriangles_refcountbuffers have their last value as 0.If it doesn't, the next required value is out of the boundaries of the array, and it fails.
Solution
Add extra boundaries check for all
whileloops working with these buffers.Also, it seems that we can skip iteration over these vectors if we know that they are compact.
Sadly, with whitespace changes VS added spaces in changed expressions.