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

Fix range loop iteration regressions #72078

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Jan 25, 2023

I think this PR (#70773) accidentally broke the vertex remapping. The old code wrote back the values (edge.vertex_a and edge.vertex_b), but the new code writes the values to a copy that is then thrown away. Haven't really fully figured how this affects the mesh optimization but I think this is a correct fix. @KoBeWi

@bitsawer bitsawer requested a review from a team as a code owner January 25, 2023 21:31
@KoBeWi KoBeWi added this to the 4.0 milestone Jan 25, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jan 25, 2023

All loops should iterate using references. Might be worth re-checking that PR if there aren't any more mistakes.

@bitsawer
Copy link
Member Author

Found another one in mesh_storage.cpp, but it looks like a inefficiency (copies insteads of references) rather than a behavior changing bug. I'll fix that too and update the PR once I have double checked everyting.

@bitsawer bitsawer force-pushed the fix_optimize_vertices branch from afd96b5 to 8c25bcd Compare January 25, 2023 22:16
@bitsawer bitsawer requested a review from a team as a code owner January 25, 2023 22:16
@bitsawer
Copy link
Member Author

Went through the PR but those two issues are the only I could spot. Updated the PR and commit message.

In one of the projects I have worked on we had a statical analyzer and rule that only allowed primitive types, references or pointers (const or not-const) in loops like that. It would also warn if a non-const loop value wasn't modified. I remember some people have run different analyzers on Godot source code in the past and reported bugs, could be useful.

@bitsawer bitsawer changed the title Fix MeshData::optimize_vertices() remapping Fix range loop iteration regressions Jan 25, 2023
@akien-mga akien-mga merged commit 31496c2 into godotengine:master Jan 26, 2023
@akien-mga
Copy link
Member

Thanks!

@bitsawer bitsawer deleted the fix_optimize_vertices branch January 26, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants