Fix #106247: Remove incorrect selection check when in multi-mesh #106350

Jake · 2023-03-31T02:16:29+02:00

Jake commented

2023-03-31 02:16:29 +02:00

object_get_datamask checks if the supplied object is the active object, causing the datamask to not update for non-active selected objects when entering multi-mesh editing. This causes the mesh data to be rebuilt unexpectedly, causing the assert fail.

The check doesn't seem to have any effect for single-mesh selections, as an object having a mode flag set seems to imply it is selected, so the only effect of the check is to cause incorrect behavior on multi-mesh selections.

`object_get_datamask` checks if the supplied object is the active object, causing the datamask to not update for non-active selected objects when entering multi-mesh editing. This causes the mesh data to be rebuilt unexpectedly, causing the assert fail. The check doesn't seem to have any effect for single-mesh selections, as an object having a mode flag set seems to imply it is selected, so the only effect of the check is to cause incorrect behavior on multi-mesh selections.

Jorijn de Graaf commented

2023-03-31 02:41:27 +02:00

I have no jurisdiction to review but as the guy who opened the issue I want to say: this looks good to me 👍

Jake requested review from Hans Goudey 2023-04-01 06:19:43 +02:00

Hans Goudey commented

2023-04-01 15:32:10 +02:00

The time complexity of this solution looks concerning. I see "for every object evaluation, loop over the original objects". We shouldn't be introducing quadratic time complexity here. Maybe I'm missing something though.

This causes the mesh data to be rebuilt unexpectedly

I think I need a bit more information here. What is wrong about how the mesh was built? Why does the missing selection check cause there to be no evaluated data?

The time complexity of this solution looks concerning. I see "for every object evaluation, loop over the original objects". We shouldn't be introducing quadratic time complexity here. Maybe I'm missing something though. >This causes the mesh data to be rebuilt unexpectedly I think I need a bit more information here. What is wrong about how the mesh was built? Why does the missing selection check cause there to be no evaluated data?

Jorijn de Graaf commented

2023-04-02 23:11:53 +02:00

I don't think it's necessary to iterate over bases, so let's get rid of this:

  bool selected = false;
  LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_get(view_layer)) {
    if ((base->flag & BASE_SELECTED) != 0 && origob == DEG_get_original_object(base->object)) {
      selected = true;
      break;
    }
  if (selected) {

And instead do this:

  if (origob->base_flag & BASE_SELECTED) {

I believe Object->base_flag is a copy of Base->flag, and I've seen it used in similar fasion elsewhere.
Object->flag appears to be the same thing, so you could also use something like this:

  if (origob->flag & SELECT) {

I'm not sure what the difference is.
These both seem to work fine, at least in my simple testing.

We'll need to hear from @HooglyBoogly what he thinks.

I don't think it's necessary to iterate over bases, so let's get rid of this: ```cc bool selected = false; LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_get(view_layer)) { if ((base->flag & BASE_SELECTED) != 0 && origob == DEG_get_original_object(base->object)) { selected = true; break; } if (selected) { ``` And instead do this: ```cc if (origob->base_flag & BASE_SELECTED) { ``` I believe Object->base_flag is a copy of Base->flag, and I've seen it used in similar fasion elsewhere. Object->flag appears to be the same thing, so you could also use something like this: ```cc if (origob->flag & SELECT) { ``` I'm not sure what the difference is. These both seem to work fine, at least in my simple testing. We'll need to hear from @HooglyBoogly what he thinks.

Jake commented

2023-04-03 04:44:46 +02:00

I'm pretty sure I made a wrong assumption somewhere, because after looking at it some more I'm not really sure what the check is for. Outside of the scenario of multiple objects selected at once in a view layer, I've yet to see any case where the presence of the (current) check actually changes the returned data mask.

I think I need a bit more information here. What is wrong about how the mesh was built? Why does the missing selection check cause there to be no evaluated data?

For the last used data mask on the active mesh, on entering edit mode the CD_MASK_MVERT_SKIN flag is set while for any other selected mesh it isn't. In the original issue, selecting a vert on the non-active object makes it active, which causes the returned data mask to be different next time it's checked, since it now passes the check. This leads to BKE_object_eval_assign_data to be called via editbmesh_get_eval_cage while runtime.eval_data is still set, failing the assert. I assumed that it's correct for both objects to be flagged when entering edit mode, since it looked like the selection check hadn't been changed since before multi-mesh editing was introduced.

I'm pretty sure I made a wrong assumption somewhere, because after looking at it some more I'm not really sure what the check is for. Outside of the scenario of multiple objects selected at once in a view layer, I've yet to see any case where the presence of the (current) check actually changes the returned data mask. > I think I need a bit more information here. What is wrong about how the mesh was built? Why does the missing selection check cause there to be no evaluated data? For the last used data mask on the active mesh, on entering edit mode the `CD_MASK_MVERT_SKIN` flag is set while for any other selected mesh it isn't. In the original issue, selecting a vert on the non-active object makes it active, which causes the returned data mask to be different next time it's checked, since it now passes the check. This leads to `BKE_object_eval_assign_data` to be called via `editbmesh_get_eval_cage` while `runtime.eval_data` is still set, failing the assert. I assumed that it's correct for both objects to be flagged when entering edit mode, since it looked like the selection check hadn't been changed since before multi-mesh editing was introduced.

Hans Goudey changed title from ~~Fix #106247 - Fix incorrect selection check when in multi-mesh~~ to Fix #106247: Fix incorrect selection check when in multi-mesh

2023-04-03 17:49:33 +02:00

Jake force-pushed Fix-106247 from 581a3111a8 to e8a706cc26

2023-04-17 19:06:06 +02:00

Compare

Jake changed title from ~~Fix #106247: Fix incorrect selection check when in multi-mesh~~ to Fix #106247: Remove incorrect selection check when in multi-mesh

2023-04-17 19:18:27 +02:00

This pull request has changes conflicting with the target branch.

source/blender/blenkernel/intern/DerivedMesh.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u Fix-106247:Jake-Faulkner-Fix-106247

git checkout Jake-Faulkner-Fix-106247

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #106247: Remove incorrect selection check when in multi-mesh #106350

Checkout