Customdata merging does not check layer limit on destination #111609

Philipp Oeser · 2023-08-28T13:54:46+02:00

Philipp Oeser commented

2023-08-28 13:54:46 +02:00

LayerTypeInfo can define a layers_max() function which determines the
maximum allowed number of layers.

Upon merging, this limit was respected from the source, but not on the
destination, so it was possible to exceed the max (if there were layers
on the destination already).

NOTE: layers_max() is currently only defined for legacy CD_MTFACE, but
we might want to enforce this for UVs / CD_PROP_FLOAT2 again.

This came up in #111608.

`LayerTypeInfo` can define a `layers_max()` function which determines the maximum allowed number of layers. Upon merging, this limit was respected from the source, but not on the destination, so it was possible to exceed the max (if there were layers on the destination already). NOTE: `layers_max()` is currently only defined for legacy CD_MTFACE, but we might want to enforce this for UVs / CD_PROP_FLOAT2 again. This came up in #111608.

Philipp Oeser added 1 commit 2023-08-28 13:54:59 +02:00

784e3949d4 Customdata merging does not check layer limit on destination

`LayerTypeInfo` can define a `layers_max()` function which determines the
maximum allowed number of layers.

Upon merging, this limit was respected from the source, but not on the
destination, so it was possible to exceed the max (if there were layers
on the destination already).

NOTE: `layers_max()` is currently only defined for legacy CD_MTFACE, but
we might want to enforce this for UVs / CD_PROP_FLOAT2 again.

This came up in #111608.

Philipp Oeser added this to the Modeling project 2023-08-28 13:55:08 +02:00

Philipp Oeser requested review from Hans Goudey 2023-08-28 13:55:17 +02:00

Philipp Oeser requested review from Campbell Barton 2023-08-28 13:55:24 +02:00

Philipp Oeser requested review from Bastien Montagne 2023-08-28 13:55:30 +02:00

Hans Goudey commented

2023-08-28 14:31:37 +02:00

Hmm, one of the points of moving to generic attributes is that we don't have this sort of limit anymore. Agree in principle a limit should be respected when merging, but since CD_MTFACE is only used for legacy MFace faces, this won't really have an effect.

Hmm, one of the points of moving to generic attributes is that we don't have this sort of limit anymore. Agree in principle a limit should be respected when merging, but since `CD_MTFACE` is only used for legacy `MFace` faces, this won't really have an effect.

Bastien Montagne approved these changes 2023-08-28 14:31:41 +02:00

Bastien Montagne left a comment

Looks reasonable to me :)

Philipp Oeser commented

2023-08-28 14:37:22 +02:00

Hmm, one of the points of moving to generic attributes is that we don't have this sort of limit anymore. Agree in principle a limit should be respected when merging, but since CD_MTFACE is only used for legacy MFace faces, this won't really have an effect.

Yes, but shouldnt this be corrected still? And I am not totally sure we never want limits going forward? For example the UV case? (have you checked on #111608 and the other issues mentioned there?)

> Hmm, one of the points of moving to generic attributes is that we don't have this sort of limit anymore. Agree in principle a limit should be respected when merging, but since `CD_MTFACE` is only used for legacy `MFace` faces, this won't really have an effect. Yes, but shouldnt this be corrected still? And I am not totally sure we **never** want limits going forward? For example the UV case? (have you checked on #111608 and the other issues mentioned there?)

Bastien Montagne commented

2023-08-28 16:49:52 +02:00

@HooglyBoogly AFAIK UI and user-level tools in Blender still limit the amount of UV layers to 8? So having code (besides very maybe python API) that allows to generate meshes with more UV layers than this limit is very, very bad.

@HooglyBoogly AFAIK UI and user-level tools in Blender still limit the amount of UV layers to 8? So having code (besides _very maybe_ python API) that allows to generate meshes with more UV layers than this limit is very, very bad.

Campbell Barton commented

2023-08-29 08:10:09 +02:00

Accepting, although I think it would be good to have object joining default to match UV layers by index instead of by name, this way joining many meshes won't loose their UV's. There can be an option to map layer names for UV's still but this has the down side that it may attempt to add more than 8 layers which then looses UV data.

Campbell Barton approved these changes 2023-08-29 08:10:21 +02:00

Philipp Oeser commented

2023-08-29 13:58:51 +02:00

Would like to clarify that this patch DOES NOT alter the behavior of the join operator at all atm. (at least not without introducing further changes such as adding layers_max() for CD_PROP_FLOAT2)

Would like to clarify that this patch DOES NOT alter the behavior of the join operator at all atm. (at least not without introducing further changes such as adding `layers_max()` for `CD_PROP_FLOAT2`)

Hans Goudey approved these changes 2023-08-29 15:24:44 +02:00

Hans Goudey left a comment

Enforcing the limit is okay. I don't think adding back a limit for UVs is the right way forward, but we can handle that separately.

Philipp Oeser merged commit a3c045d49d into main

2023-08-29 18:02:44 +02:00

Philipp Oeser referenced this issue from a commit

2023-08-29 18:02:44 +02:00

Fix: Customdata merging does not check layer limit on destination

Philipp Oeser deleted branch 111608_a

2023-08-29 18:02:46 +02:00

Stefan Werner referenced this issue from a commit

2023-09-07 15:02:36 +02:00