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

Infer array length from custom array if no vertex array is provided #150

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Adrian-Samoticha
Copy link

I am unsure if PRs that haven’t been merged into Godot are already being considered, but if yes, then this is the Godot PR:
godotengine/godot#93549

Adrian-Samoticha and others added 3 commits June 24, 2024 14:37
While the `RenderingServer::mesh_create_surface_data_from_arrays`
method does support vertexless meshes (see godotengine#62046 and godotengine#83446),
it enforces that the size of custom arrays is dependent on the size of
the vertex array. This effectively means  that custom arrays cannot be
used in vertexless meshes.

This commit changes the way the array length is computed so that if no
vertex array is provided, its length will be inferred from the custom
arrays, if provided. It therefore adds support for custom arrays in
vertexless meshes.
Co-authored-by: A Thousand Ships (she/her) <96648715+AThousandShips@users.noreply.github.com>
if (array_len == 0) {
Vector<Variant> custom_array = p_arrays[RS::ARRAY_CUSTOM0 + i];
uint32_t factor_reciprocal = _get_vertex_to_custom_array_length_factor(format, RS::ARRAY_CUSTOM0 + i);
array_len = custom_array.size() / factor_reciprocal;

Choose a reason for hiding this comment

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

I know it's probably not a problem, but is there any chance custom_array.size() could return 0?

Copy link
Author

Choose a reason for hiding this comment

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

Well, ARRAY_CUSTOM0 can be any size, but if it’s 0, then the code should just behave the same way it does without my changes.

Choose a reason for hiding this comment

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

The thing is, in C++, dividing by 0 is undefined. So there is a chance that doing this could introduce garbage into the variable that can screw things up. Having a check before the division to make sure that it's not 0 and bail if it is will prevent the garbage from happening.

Copy link
Author

Choose a reason for hiding this comment

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

I’m not dividing by custom_array.size(), though. custom_array.size() is the dividend in this case. I’m dividing by factor_reciprocal which shouldn’t ever be 0.

Choose a reason for hiding this comment

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

I used the wrong verbage, but the problem still remains. It's undefined behavior in C++, which could cause garbage values, even if the dividend is 0.

The code and the idea itself is good, I just feel like having a custom_array.size() != 0 check before dividing would fix a potential problem source from undefined behavior and the dark magic that is the C++ compilers.

Copy link

Choose a reason for hiding this comment

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

I used the wrong verbage, but the problem still remains. It's undefined behavior in C++, which could cause garbage values, even if the dividend is 0.

The code and the idea itself is good, I just feel like having a custom_array.size() != 0 check before dividing would fix a potential problem source from undefined behavior and the dark magic that is the C++ compilers.

Dividing 0 by any integer other than 0 is well defined behaviour in C++ or any other language for that matter.
Checking the dividend for 0 would be pointless.

Choose a reason for hiding this comment

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

After educating myself farther, I was wrong. I'm sorry about that.

@Spartan322
Copy link
Member

Spartan322 commented Oct 14, 2024

This needs to be squashed.

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.

5 participants