Correctly setting active UV maps during "Realize Instances" #116452

Merged

Jacques Lucke merged 3 commits from Eugene-Kuznetsov/blender:ek_uv_map_geonodes into main

2024-02-01 19:07:52 +01:00

Eugene-Kuznetsov commented

2023-12-21 23:06:11 +01:00

Contributor

As currently implemented, Blender sometimes incorrectly sets active/render UV maps during the "realize instances" operation. This also comes into play if you try to modify the mesh with geonodes: e.g., if you use geonodes to split a mesh into two pieces, transform one of them, and then "join geometry", join operation internally performs "realize instances", triggering the same bug. I believe it's the same bug as reported in #112181 and possibly some other issues. See attached blend file for a reproducer.

The root cause is that "realize instances" copies both UV map layers and the active/render layer index, but it reorders layers (possibly in alphabetical order?) during this process, so the index that was correct in the original mesh is now wrong in the modified mesh.

This patch corrects indexes by explicitly checking layer names.

As currently implemented, Blender sometimes incorrectly sets active/render UV maps during the "realize instances" operation. This also comes into play if you try to modify the mesh with geonodes: e.g., if you use geonodes to split a mesh into two pieces, transform one of them, and then "join geometry", join operation internally performs "realize instances", triggering the same bug. I believe it's the same bug as reported in https://projects.blender.org/blender/blender/issues/112181 and possibly some other issues. See attached blend file for a reproducer. The root cause is that "realize instances" copies both UV map layers and the active/render layer index, but it reorders layers (possibly in alphabetical order?) during this process, so the index that was correct in the original mesh is now wrong in the modified mesh. This patch corrects indexes by explicitly checking layer names.

geonode_uv_maps.blend

1.6 MiB

👍 1 ❤️ 1

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from 5daf20ce3d to 3ddb84f7a3

2023-12-22 05:01:56 +01:00

Compare

Iliya Katushenock added this to the Nodes & Physics project 2023-12-22 08:25:23 +01:00

Iliya Katushenock added the

label 2023-12-22 08:25:27 +01:00

Iliya Katushenock commented

2023-12-22 08:27:54 +01:00

Member

Not sure if index is make sense at all, it is not a string of activa layer currently?

Not sure if index is make sense at all, it is not a string of activa layer currently?

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from 3ddb84f7a3 to cab84e19cc

2023-12-22 23:06:52 +01:00

Compare

Eugene-Kuznetsov commented

2023-12-22 23:08:42 +01:00

Author

Contributor

The same bug independently affects "Separate Geometry" and "Delete Geometry" (reproducer attached). I've pushed an updated fix that covers those cases as well.

The same bug independently affects "Separate Geometry" and "Delete Geometry" (reproducer attached). I've pushed an updated fix that covers those cases as well.

geonode_uv_maps1.blend

1.6 MiB

Brecht Van Lommel requested review from Jacques Lucke 2024-01-12 15:45:16 +01:00

Brecht Van Lommel requested review from Hans Goudey 2024-01-12 15:45:17 +01:00

Jacques Lucke requested changes 2024-01-16 15:38:18 +01:00

source/blender/geometry/intern/realize_instances.cc Outdated

						
				@ -1172,6 +1172,13 @@ static void execute_realize_mesh_tasks(const RealizeInstancesOptions &options,

				        dst_attributes.lookup_or_add_for_write_only_span(attribute_id, domain, data_type));

				  }

				  CustomData_set_layer_active(&dst_mesh->corner_data, CD_PROP_FLOAT2,

Jacques Lucke commented

2024-01-16 15:37:59 +01:00

Member

It seems like some error handling is missing here for the case when there is no active/render attribute. Or am I missing something?

It seems like some error handling is missing here for the case when there is no active/render attribute. Or am I missing something?

Eugene-Kuznetsov commented

2024-01-16 16:53:22 +01:00

Author

Contributor

Is it even possible for a mesh to have no active/render attribute?

If the mesh has no CD_PROP_FLOAT2 layers, CustomData_get_active_layer_name calls CustomData_get_active_layer_index and receives -1, it returns nullptr, CustomData_get_named_layer returns -1, and CustomData_set_layer_active iterates through layers, finds no CD_PROP_FLOAT2 layers and therefore does nothing.

If it does have CD_PROP_FLOAT2 layers but somehow none of them are marked as active/render (layers[layer_index].active set to -1 in https://projects.blender.org/blender/blender/src/branch/main/source/blender/blenkernel/intern/customdata.cc#L2611), then there might be a problem because there will be a strcmp with null inside CustomData_get_named_layer_index. It might be cleaner to fix this by
`@@ -2584,6 +2584,9 @@ int CustomData_get_named_layer_index(const CustomData *data,
const eCustomDataType type,
const char *name)
{

if(name == nullptr)
return -1;
for (int i = 0; i < data->totlayer; i++) {
if (data->layers[i].type == type) {
if (STREQ(data->layers[i].name, name)) {
`

Is it even possible for a mesh to have no active/render attribute? If the mesh has no CD_PROP_FLOAT2 layers, CustomData_get_active_layer_name calls CustomData_get_active_layer_index and receives -1, it returns nullptr, CustomData_get_named_layer returns -1, and CustomData_set_layer_active iterates through layers, finds no CD_PROP_FLOAT2 layers and therefore does nothing. If it does have CD_PROP_FLOAT2 layers but somehow none of them are marked as active/render (layers[layer_index].active set to -1 in https://projects.blender.org/blender/blender/src/branch/main/source/blender/blenkernel/intern/customdata.cc#L2611), then there might be a problem because there will be a strcmp with null inside CustomData_get_named_layer_index. It might be cleaner to fix this by `@@ -2584,6 +2584,9 @@ int CustomData_get_named_layer_index(const CustomData *data, const eCustomDataType type, const char *name) { + if(name == nullptr) + return -1; + for (int i = 0; i < data->totlayer; i++) { if (data->layers[i].type == type) { if (STREQ(data->layers[i].name, name)) { `

Jacques Lucke commented

2024-01-18 10:58:58 +01:00

Member

I don't think that CustomData_set_layer_active is intentionally written in a way that would allow passing in -1. There should probably be an assert at some point.

I'd prefer more explicit handling of this case on the call-site.

I don't think that `CustomData_set_layer_active` is intentionally written in a way that would allow passing in `-1`. There should probably be an assert at some point. I'd prefer more explicit handling of this case on the call-site.

Eugene-Kuznetsov marked this conversation as resolved

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from cab84e19cc to ffc146addc

2024-01-16 17:31:00 +01:00

Compare

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from ffc146addc to a989813e2d

2024-01-23 22:19:25 +01:00

Compare

Jacques Lucke approved these changes 2024-01-26 11:09:46 +01:00

Jacques Lucke left a comment

Member

Generally looks good now. Needs one small cleanup.

Generally looks good now. Needs one small cleanup.

source/blender/blenkernel/intern/customdata.cc Outdated

						
				@ -2584,6 +2584,9 @@ int CustomData_get_named_layer_index(const CustomData *data,

				                                     const eCustomDataType type,

				                                     const char *name)

				{

				  if(name == nullptr)

Jacques Lucke commented

2024-01-26 11:08:23 +01:00

Member

This change seems unnecssary.

This change seems unnecssary.

Eugene-Kuznetsov marked this conversation as resolved

Jacques Lucke commented

2024-01-26 11:11:04 +01:00

Member

@blender-bot build

@blender-bot build

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from a989813e2d to 3760a9522a

2024-01-26 23:29:28 +01:00

Compare

Jacques Lucke requested changes 2024-01-29 10:50:48 +01:00

Jacques Lucke left a comment

Member

I'm sorry to say it, but this is actually not quite working yet and is failing tests (also because of the removed check that I requested earlier, but I still think it's correct that it is removed).

The reason tests are failing is that it can happen that dst_mesh does not have the attribute that's active on first_mesh. Might be reasonable to add a new function like CustomData_copy_active/render_layer that handles the different failure cases. I can understand if you are not motivated to work on this patch anymore. I can finish it up myself if you want.

The following tests FAILED:
	176 - geo_node_curves_sample_curves (Subprocess aborted)
	193 - geo_node_geometry_delete_geometry (Subprocess aborted)

I'm sorry to say it, but this is actually not quite working yet and is failing tests (also because of the removed check that I requested earlier, but I still think it's correct that it is removed). The reason tests are failing is that it can happen that `dst_mesh` does not have the attribute that's active on `first_mesh`. Might be reasonable to add a new function like `CustomData_copy_active/render_layer` that handles the different failure cases. I can understand if you are not motivated to work on this patch anymore. I can finish it up myself if you want. ``` The following tests FAILED: 176 - geo_node_curves_sample_curves (Subprocess aborted) 193 - geo_node_geometry_delete_geometry (Subprocess aborted) ```

Eugene-Kuznetsov force-pushed ek_uv_map_geonodes from 3760a9522a to ff7980c710

2024-01-30 20:30:48 +01:00

Compare

Jacques Lucke added 2 commits 2024-02-01 14:59:18 +01:00

9d043c561b Merge branch 'main' into ek_uv_map_geonodes

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

1da34fc177 fix include after merge

Jacques Lucke commented

2024-02-01 14:59:27 +01:00

Member

@blender-bot build

@blender-bot build

Jacques Lucke merged commit 4024614ecc into main

2024-02-01 19:07:52 +01:00

Jacques Lucke referenced this issue from a commit

2024-02-01 19:07:53 +01:00

Fix: correctly set active/render UV maps when realizing instances

YimingWu referenced this issue from a commit

2024-02-05 10:56:05 +01:00

Fix: correctly set active/render UV maps when realizing instances

Jonas Dichelle referenced this issue from a commit

2024-02-08 17:29:07 +01:00

Fix: correctly set active/render UV maps when realizing instances

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116452

No description provided.