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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72d84e8
Infer array length from custom array if no vertex array is provided
Adrian-Samoticha Jun 24, 2024
b5e969e
Update servers/rendering_server.cpp
Adrian-Samoticha Jun 24, 2024
8b46e58
Change error message when receiving invalid custom format type in `_g…
Adrian-Samoticha Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions servers/rendering_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1239,6 +1239,13 @@ Error RenderingServer::mesh_create_surface_data_from_arrays(SurfaceData *r_surfa
// Include custom array format type.
if (format & (1ULL << (ARRAY_CUSTOM0 + i))) {
format |= (RS::ARRAY_FORMAT_CUSTOM_MASK << (RS::ARRAY_FORMAT_CUSTOM_BASE + i * RS::ARRAY_FORMAT_CUSTOM_BITS)) & p_compress_format;

// If the mesh contains no vertex array, infer the array length from the custom array.
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;
Copy link

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.

Copy link

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.

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.

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.

Copy link

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.

}
}
}

Expand Down Expand Up @@ -1374,6 +1381,31 @@ Error RenderingServer::mesh_create_surface_data_from_arrays(SurfaceData *r_surfa
return OK;
}

int32_t RenderingServer::_get_vertex_to_custom_array_length_factor(uint32_t p_format, int p_array_index) {
uint32_t type = (p_format >> (ARRAY_FORMAT_CUSTOM_BASE + ARRAY_FORMAT_CUSTOM_BITS * (p_array_index - RS::ARRAY_CUSTOM0))) & ARRAY_FORMAT_CUSTOM_MASK;
switch (type) {
case ARRAY_CUSTOM_RGBA8_UNORM:
case ARRAY_CUSTOM_RGBA8_SNORM:
case ARRAY_CUSTOM_RG_HALF: {
return 4;
} break;
case ARRAY_CUSTOM_RGBA_HALF: {
return 8;
} break;
case ARRAY_CUSTOM_R_FLOAT:
case ARRAY_CUSTOM_RG_FLOAT:
case ARRAY_CUSTOM_RGB_FLOAT:
case ARRAY_CUSTOM_RGBA_FLOAT: {
int32_t s = type - ARRAY_CUSTOM_R_FLOAT + 1;

return s;
} break;
default: {
ERR_FAIL_V_MSG(0, "Invalid custom format type.");
}
}
}

void RenderingServer::mesh_add_surface_from_arrays(RID p_mesh, PrimitiveType p_primitive, const Array &p_arrays, const Array &p_blend_shapes, const Dictionary &p_lods, BitField<ArrayFormat> p_compress_format) {
SurfaceData sd;
Error err = mesh_create_surface_data_from_arrays(&sd, p_primitive, p_arrays, p_blend_shapes, p_lods, p_compress_format);
Expand Down
1 change: 1 addition & 0 deletions servers/rendering_server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ class RenderingServer : public Object {
/// Returns stride
virtual void mesh_surface_make_offsets_from_format(uint64_t p_format, int p_vertex_len, int p_index_len, uint32_t *r_offsets, uint32_t &r_vertex_element_size, uint32_t &r_normal_element_size, uint32_t &r_attrib_element_size, uint32_t &r_skin_element_size) const;
virtual Error mesh_create_surface_data_from_arrays(SurfaceData *r_surface_data, PrimitiveType p_primitive, const Array &p_arrays, const Array &p_blend_shapes = Array(), const Dictionary &p_lods = Dictionary(), uint64_t p_compress_format = 0);
int32_t _get_vertex_to_custom_array_length_factor(uint32_t p_format, int p_array_index);
Array mesh_create_arrays_from_surface_data(const SurfaceData &p_data) const;
Array mesh_surface_get_arrays(RID p_mesh, int p_surface) const;
TypedArray<Array> mesh_surface_get_blend_shape_arrays(RID p_mesh, int p_surface) const;
Expand Down
Loading