blender
/
blender
129
397
Fork 571
Code Issues 6.2k Pull Requests 752 Projects 19 Wiki Activity

Python API: Mesh editing: Editing uvs after normals with a previously assigned variable doesn't work #107500

New Issue
Open
opened 2023-05-01 16:40:09 +02:00 by Dragorn421 · 35 comments
Dragorn421 commented 2023-05-01 16:40:09 +02:00
Copy Link

Blender Version
Broken: 3.5.1
Worked: 3.4.1

Caused by 6c774feba2

Short description of error
Holding on a variable referencing the uv layer data for later uv editing, doesn't work when normals are edited in between

Exact steps for others to reproduce the error
Run the attached python script in Blender 3.5.1
This creates three objects, all should have all their loop uvs as 0,0:

  • no_set_normals (should have default normals, sideways)
  • with_set_normals_and_reaccess_uv (should have upwards +z normals)
  • with_set_normals_but_no_reaccess_uv (should have upwards +z normals)

Go in edit mode on each generated object, look at the normals and uvs
The results are only correct for no_set_normals and with_set_normals_and_reaccess_uv
For with_set_normals_but_no_reaccess_uv, neither the uvs nor normals are correct. Both appear to be defaults

In Blender 3.4.1 this works as expected, i.e. with_set_normals_and_reaccess_uv and with_set_normals_but_no_reaccess_uv are identical

Attached are also screenshots in 3.5.1 of the normals and uvs of each object
And a screenshot in 3.4.1 showing the expected results

Guess at what's happening
On more complicated cases (actual meshes with hundreds of triangles), the normals are messed up (seemingly random instead of defaults) and Blender is prone to crashing. This leads me to believe holding on the uv layer data after normal editing accesses freed memory or something like that?

**Blender Version** Broken: 3.5.1 Worked: 3.4.1 Caused by 6c774feba2 **Short description of error** Holding on a variable referencing the uv layer data for later uv editing, doesn't work when normals are edited in between **Exact steps for others to reproduce the error** Run the attached python script in Blender 3.5.1 This creates three objects, all should have all their loop uvs as 0,0: - no_set_normals (should have default normals, sideways) - with_set_normals_and_reaccess_uv (should have upwards +z normals) - with_set_normals_but_no_reaccess_uv (should have upwards +z normals) Go in edit mode on each generated object, look at the normals and uvs The results are only correct for no_set_normals and with_set_normals_and_reaccess_uv For with_set_normals_but_no_reaccess_uv, neither the uvs nor normals are correct. Both appear to be defaults In Blender 3.4.1 this works as expected, i.e. with_set_normals_and_reaccess_uv and with_set_normals_but_no_reaccess_uv are identical Attached are also screenshots in 3.5.1 of the normals and uvs of each object And a screenshot in 3.4.1 showing the expected results **Guess at what's happening** On more complicated cases (actual meshes with hundreds of triangles), the normals are messed up (seemingly random instead of defaults) and Blender is prone to crashing. This leads me to believe holding on the uv layer data after normal editing accesses freed memory or something like that?
no_set_normals.png
412 KiB
with_set_normals_and_reaccess_uv.png
418 KiB
3_4_1.png
461 KiB
test uv normals.py
1.1 KiB
with_set_normals_but_no_reaccess_uv.png
419 KiB
no_set_normals.png with_set_normals_and_reaccess_uv.png 3_4_1.png with_set_normals_but_no_reaccess_uv.png
Dragorn421 added the
Priority
Normal
Type
Report
Status
Needs Triage
 labels 2023-05-01 16:40:10 +02:00
Iliya Katushenock commented 2023-05-01 16:45:12 +02:00
Member
Copy Link

Will check

Will check
Iliya Katushenock commented 2023-05-01 17:29:20 +02:00
Member
Copy Link

I can get exception in debug build, but without stack trace. Just to be sure, should i run script with selected default cube?

I can get exception in debug build, but without stack trace. Just to be sure, should i run script with selected default cube?
Iliya Katushenock commented 2023-05-01 18:01:57 +02:00
Member
Copy Link
    uv_layer = mesh.uv_layers.new()
    uv_layer_data = uv_layer.data
    uv_layer_name = uv_layer.name
    del uv_layer

Hey, in this code you didn't create real copy of attribute. Do, after your delition, you continue to use freed memory?

```Py uv_layer = mesh.uv_layers.new() uv_layer_data = uv_layer.data uv_layer_name = uv_layer.name del uv_layer ``` Hey, in this code you didn't create real copy of attribute. Do, after your delition, you continue to use freed memory?
Dragorn421 commented 2023-05-01 19:02:05 +02:00
Author
Copy Link

I can get exception in debug build, but without stack trace. Just to be sure, should i run script with selected default cube?

The script is used to show the issue by generating three mesh objects. The issue is then with those mesh objects. You don't need to be in a specific state to run it or have anything selected in particular.

It runs fine (without raising exceptions), in both 3.4.1 and 3.5.1, as expected. If you're getting an exception, it's not what I intend to demonstrate here

    uv_layer = mesh.uv_layers.new()
    uv_layer_data = uv_layer.data
    uv_layer_name = uv_layer.name
    del uv_layer

Hey, in this code you didn't create real copy of attribute. Do, after your delition, you continue to use freed memory?

I don't understand the question
del x does not free memory in python if that's what you're refering to, it merely unbinds the variable from the scope. I put that line to avoid confusion between uv_layer and uv_layer_data, but you can remove it if you want

> I can get exception in debug build, but without stack trace. Just to be sure, should i run script with selected default cube? The script is used to show the issue by generating three mesh objects. The issue is then with those mesh objects. You don't need to be in a specific state to run it or have anything selected in particular. It runs fine (without raising exceptions), in both 3.4.1 and 3.5.1, as expected. If you're getting an exception, it's not what I intend to demonstrate here > ```Py > uv_layer = mesh.uv_layers.new() > uv_layer_data = uv_layer.data > uv_layer_name = uv_layer.name > del uv_layer > ``` > Hey, in this code you didn't create real copy of attribute. Do, after your delition, you continue to use freed memory? > I don't understand the question `del x` does not free memory in python if that's what you're refering to, it merely unbinds the variable from the scope. I put that line to avoid confusion between uv_layer and uv_layer_data, but you can remove it if you want
Iliya Katushenock commented 2023-05-01 19:04:02 +02:00
Member
Copy Link

Ah, i see, well, i cannot check this due to i getting really a lot of exception related with segfault.

Ah, i see, well, i cannot check this due to i getting really a lot of exception related with segfault.
Iliya Katushenock added the
Interest
Modeling
Interest
Python API
 labels 2023-05-01 19:04:13 +02:00
Dragorn421 commented 2023-05-01 19:11:55 +02:00
Author
Copy Link

Could be linked to the issue in #107168

Could be linked to the issue in #107168
YimingWu commented 2023-05-02 07:52:57 +02:00
Member
Copy Link

This leads me to believe holding on the uv layer data after normal editing accesses freed memory or something like that?

I believe you can't really hold that pointer "for later" but from the script it looks like should be fine. You only changed normal and later accessed the uv layer with ID, so can't really think of an obvious cause.

This I think involves the copy-on-write mechanism, but not sure if current python API takes care of that already...

> This leads me to believe holding on the uv layer data after normal editing accesses freed memory or something like that? I believe you can't really hold that pointer "for later" but from the script it looks like should be fine. You only changed normal and later accessed the uv layer with ID, so can't really think of an obvious cause. This I think involves the copy-on-write mechanism, but not sure if current python API takes care of that already...
Thomas Barlow commented 2023-05-02 23:28:33 +02:00
Member
Copy Link

It looks to me like the existing references to uv_layer are becoming invalidated after Mesh.normals_split_custom_set is called. I'm guessing this also extends to existing references to uv_layer.data, but I can't tell since it doesn't have as_pointer().

It does look similar to #107168, though this time it's because of Mesh.normals_split_custom_set.

I did wonder if this was happening because Mesh.normals_split_custom_set creates the "sharp_edge" attribute in 3.5.1+, but I added the "sharp_edge" attribute before even creating uv_layer and still got the same result.

import bpy

me = bpy.data.meshes.new("")

# Creating "sharp_edge" attribute in advance does not affect the result
#if bpy.app.version >= (3, 5):
#    me.attributes.new("sharp_edge", 'BOOLEAN', 'EDGE')

uv_layer = me.uv_layers.new(name="UV")
uv_layer_name = uv_layer.name
uv_layer_pointer = uv_layer.as_pointer()

uv_layer_from_mesh = me.uv_layers[uv_layer_name]

print("Before normals_split_custom_set:")
print(uv_layer_name, uv_layer.name, uv_layer_from_mesh.name, sep=", ")
print(uv_layer_pointer, uv_layer.as_pointer(), uv_layer_from_mesh.as_pointer(), sep=", ")

me.normals_split_custom_set([])

uv_layer_from_mesh = me.uv_layers[uv_layer_name]

print("After normals_split_custom_set:")
print(uv_layer_name, uv_layer.name, uv_layer_from_mesh.name, sep=", ")
print(uv_layer_pointer, uv_layer.as_pointer(), uv_layer_from_mesh.as_pointer(), sep=", ")

# Fails in 3.5.1 and 3.6.0a 2023-05-01 0652945dbda7
assert uv_layer.as_pointer() == uv_layer_from_mesh.as_pointer()

Example console output on 3.5.1 and 3.6.0.a 2023-05-01 0652945dbda7 (on Windows 10):

Before normals_split_custom_set:
UV, UV, UV
2288366692744, 2288366692744, 2288366692744
After normals_split_custom_set:
UV, , UV
2288366692744, 2288366692744, 2288366692864
Error: Python: Traceback (most recent call last):
  File "\Text", line 28, in <module>
AssertionError

Example console output on 3.4.1:

Before normals_split_custom_set:
UV, UV, UV
1804750480520, 1804750480520, 1804750480520
After normals_split_custom_set:
UV, UV, UV
1804750480520, 1804750480520, 1804750480520
It looks to me like the existing references to `uv_layer` are becoming invalidated after `Mesh.normals_split_custom_set` is called. I'm guessing this also extends to existing references to `uv_layer.data`, but I can't tell since it doesn't have `as_pointer()`. It does look similar to #107168, though this time it's because of `Mesh.normals_split_custom_set`. I did wonder if this was happening because `Mesh.normals_split_custom_set` creates the "sharp_edge" attribute in 3.5.1+, but I added the "sharp_edge" attribute before even creating `uv_layer` and still got the same result. ```py import bpy me = bpy.data.meshes.new("") # Creating "sharp_edge" attribute in advance does not affect the result #if bpy.app.version >= (3, 5): # me.attributes.new("sharp_edge", 'BOOLEAN', 'EDGE') uv_layer = me.uv_layers.new(name="UV") uv_layer_name = uv_layer.name uv_layer_pointer = uv_layer.as_pointer() uv_layer_from_mesh = me.uv_layers[uv_layer_name] print("Before normals_split_custom_set:") print(uv_layer_name, uv_layer.name, uv_layer_from_mesh.name, sep=", ") print(uv_layer_pointer, uv_layer.as_pointer(), uv_layer_from_mesh.as_pointer(), sep=", ") me.normals_split_custom_set([]) uv_layer_from_mesh = me.uv_layers[uv_layer_name] print("After normals_split_custom_set:") print(uv_layer_name, uv_layer.name, uv_layer_from_mesh.name, sep=", ") print(uv_layer_pointer, uv_layer.as_pointer(), uv_layer_from_mesh.as_pointer(), sep=", ") # Fails in 3.5.1 and 3.6.0a 2023-05-01 0652945dbda7 assert uv_layer.as_pointer() == uv_layer_from_mesh.as_pointer() ``` Example console output on 3.5.1 and 3.6.0.a 2023-05-01 `0652945dbda7` (on Windows 10): ``` Before normals_split_custom_set: UV, UV, UV 2288366692744, 2288366692744, 2288366692744 After normals_split_custom_set: UV, , UV 2288366692744, 2288366692744, 2288366692864 Error: Python: Traceback (most recent call last): File "\Text", line 28, in <module> AssertionError ``` Example console output on 3.4.1: ``` Before normals_split_custom_set: UV, UV, UV 1804750480520, 1804750480520, 1804750480520 After normals_split_custom_set: UV, UV, UV 1804750480520, 1804750480520, 1804750480520 ```
🚀 1
Pratik Borhade commented 2023-05-03 12:28:28 +02:00
Member
Copy Link

I can confirm. Script looks correct to me. Will check which commit has changed it.

I can confirm. Script looks correct to me. Will check which commit has changed it.
Pratik Borhade added
Module
Modeling
Status
Confirmed
 and removed
Status
Needs Triage
 labels 2023-05-03 12:28:44 +02:00
Pratik Borhade commented 2023-05-03 13:23:12 +02:00
Member
Copy Link

Caused by 6c774feba2
@Baardaap ^

Caused by 6c774feba2c9a1eb5834646f597a0f2c63177914 @Baardaap ^
Pratik Borhade added
Priority
High
 and removed
Priority
Normal
 labels 2023-05-04 10:22:31 +02:00
Martijn Versteegh commented 2023-05-05 22:18:18 +02:00
Member
Copy Link

just back from a short holiday. will try to look into it

just back from a short holiday. will try to look into it
👍 2
Martijn Versteegh self-assigned this 2023-05-05 22:18:32 +02:00
Martijn Versteegh commented 2023-05-07 01:07:20 +02:00
Member
Copy Link

I did some preliminary research. And I fear this is something that 'accidentally worked' in previous versions because the MeshUVLoopLayer used in the python api is just a wrapped pointer to the CustomDataLayer. But the CustomDataLayer's address changes willy-nilly every time layers are added or removed.

Because the CustomData layers are stored sorted by type id, and the old CD_MLOOPUV type happened to have id 16, which is quite low, the chance of layers being inserted before the UV layers was not very large (but certainly not zero!) . The new uv layers are stored as CD_FLOAT2 layers, which corresponds to id 49. CD_CUSTOMLOOPNORMAL is 41, so any custom normals added here will cause the CustomDataLayers for the UV's get shifted up.

I'm not really sure how to fix this. I guess the RNA_MeshUVLoopLayer type should not wrap a CustomDataLayer *, but reference the UV layer by name or something like that. But I'm not really well versed enough in the python api stuff to know a quick fix. I'll do some more research.

I did some preliminary research. And I fear this is something that 'accidentally worked' in previous versions because the MeshUVLoopLayer used in the python api is just a wrapped pointer to the CustomDataLayer. But the CustomDataLayer's address changes willy-nilly every time layers are added or removed. Because the CustomData layers are stored sorted by type id, and the old CD_MLOOPUV type happened to have id 16, which is quite low, the chance of layers being inserted *before* the UV layers was not very large (but certainly not zero!) . The new uv layers are stored as CD_FLOAT2 layers, which corresponds to id 49. CD_CUSTOMLOOPNORMAL is 41, so any custom normals added here will cause the CustomDataLayers for the UV's get shifted up. I'm not really sure how to fix this. I guess the RNA_MeshUVLoopLayer type should not wrap a CustomDataLayer *, but reference the UV layer by name or something like that. But I'm not really well versed enough in the python api stuff to know a quick fix. I'll do some more research.
👍 1
Martijn Versteegh commented 2023-05-09 14:00:21 +02:00
Member
Copy Link

After more studying I've concluded that , unfortunate as it is, it is not really feasible to fix this in a backwards compatible manner. So I propose to close this as a 'known limitation' and to overhaul the uv (and attribute, which suffers from the same problem) access api for 4.0 in a breaking way.

I created a devtalk topic to discuss this: https://devtalk.blender.org/t/python-api-access-of-customdatalayer-cant-handle-inserted-layers/

I'd like an opinion on closing this as 'known limitation' from the core module or the python api module. Maybe @ideasman42 or @brecht can weigh in on this?

I add a braindump fo my thinking about this problem below:

I've thought of various baroque schemes to work around this, but none that really seems feasible.

  • the simplest 'fix' would be allocating an extra table of pointers which stays stable which point to the actual CustomDataLayers , it could be kept up to date in CustomData_update_typemap. The problem with this is that it would need to be large enough to keep all possible layers, and allocated in one go because reallocing would defeat the purpose. Since I don't think there is an actual maximum this solution is out.
  • another 'fix' would be to have PointerRNA.data point to the name of the needed layer, and look up the correct layer in all accessor functions. The problem with this is that we can't use CustomDataLayer.name for this, as it gets changed when moving around the layers. We would need to have some permanent store of names somewhere. Also I don't really grasp all the machinery of the python struct callbacks and how to override those. This seems way to complicated imho.
After more studying I've concluded that , unfortunate as it is, it is not really feasible to fix this in a backwards compatible manner. So I propose to close this as a 'known limitation' and to overhaul the uv (and attribute, which suffers from the same problem) access api for 4.0 in a breaking way. I created a devtalk topic to discuss this: https://devtalk.blender.org/t/python-api-access-of-customdatalayer-cant-handle-inserted-layers/ I'd like an opinion on closing this as 'known limitation' from the core module or the python api module. Maybe @ideasman42 or @brecht can weigh in on this? I add a braindump fo my thinking about this problem below: I've thought of various baroque schemes to work around this, but none that really seems feasible. - the simplest 'fix' would be allocating an extra table of pointers which stays stable which point to the actual CustomDataLayers , it could be kept up to date in CustomData_update_typemap. The problem with this is that it would need to be large enough to keep all possible layers, and allocated in one go because reallocing would defeat the purpose. Since I don't think there is an actual maximum this solution is out. - another 'fix' would be to have PointerRNA.data point to the *name* of the needed layer, and look up the correct layer in all accessor functions. The problem with this is that we can't use CustomDataLayer.name for this, as it gets changed when moving around the layers. We would need to have some permanent store of names somewhere. Also I don't really grasp all the machinery of the python struct callbacks and how to override those. This seems way to complicated imho.
Philipp Oeser removed the
Interest
Modeling
 label 2023-05-09 15:04:11 +02:00
Martijn Versteegh commented 2023-05-09 15:53:17 +02:00
Member
Copy Link

Hm. seeing #107168 as well this might turn out to be more of a problem than I had hoped. So maybe we need to magic with the python api callbacks after all :-(.

Hm. seeing #107168 as well this might turn out to be more of a problem than I had hoped. So maybe we need to magic with the python api callbacks after all :-(.
Martijn Versteegh commented 2023-05-17 22:37:24 +02:00
Member
Copy Link

I've been trying various approaches to fix this during last week. So far unsuccessful. My current approach
might work, though I'm not yet in a state that I can really test it. It involves

  • adding a table of CustomDataLayer * to the CustomData struct (layer_locator).
  • adding a field in the CustomDataLayer struct which indexes into above table
  • making the PointerRNA.ptr point to an entry in the layer_locator table
  • make all code which uses PointerRNA.ptr dereference it to get the CustomDataLayer *
  • write bespoke iterator_begin, iterator_next, iterator_end, iterator_get functions for MeshUVLoopLayer
  • find a way to store the layertype in the CollectionPropertyIterator, which means allocating a struct in iterator_begin and freeing it again in iterator_end I guess?

All this gets reasonably complex and baroque, and I'm not really sure if there's extra parts of blenders rna code which would expect PointerRNA.ptr to point directly to the exported struct instead of indirect via an extra lookup.

So before I plod on and try to get this to work I'd like some consensus if it's even wanted, or if we decide to just document this as a known problem for 3.5 and 3.6 and concentrate on designing a new (incompatible) attribute (and uv) interface for 4.0.

I've been trying various approaches to fix this during last week. So far unsuccessful. My current approach might work, though I'm not yet in a state that I can really test it. It involves - adding a table of CustomDataLayer * to the CustomData struct (layer_locator). - adding a field in the CustomDataLayer struct which indexes into above table - making the PointerRNA.ptr point to an entry in the layer_locator table - make all code which uses PointerRNA.ptr dereference it to get the CustomDataLayer * - write bespoke iterator_begin, iterator_next, iterator_end, iterator_get functions for MeshUVLoopLayer - find a way to store the layertype in the CollectionPropertyIterator, which means allocating a struct in iterator_begin and freeing it again in iterator_end I guess? All this gets reasonably complex and baroque, and I'm not really sure if there's extra parts of blenders rna code which would expect PointerRNA.ptr to point directly to the exported struct instead of indirect via an extra lookup. So before I plod on and try to get this to work I'd like some consensus if it's even wanted, or if we decide to just document this as a *known problem* for 3.5 and 3.6 and concentrate on designing a new (incompatible) attribute (and uv) interface for 4.0.
Hans Goudey commented 2023-05-18 00:35:38 +02:00
Member
Copy Link

To me this seems like a common problem with RNA types backed by non-data-block DNA types. It's hard to guarantee pointer stability for them, and it's often not the right choice. Personally I'd rather RNA be able to handle attributes in a way that's less tied to the internal CustomDataLayer pointer. That may be tricky, but it could be handled something like this:

struct RNAAttribute {
   Mesh/Curve/Points/etc. *owner_id;
   string attribute_name;
};

That attribute struct could be owned completely by RNA (not sure this is possible right now though).
It could have attributes like AttributeReader and AttributeWriter from the C++ currently do.

To me this seems like a common problem with RNA types backed by non-data-block DNA types. It's hard to guarantee pointer stability for them, and it's often not the right choice. Personally I'd rather RNA be able to handle attributes in a way that's less tied to the internal `CustomDataLayer` pointer. That may be tricky, but it could be handled something like this: ```C struct RNAAttribute { Mesh/Curve/Points/etc. *owner_id; string attribute_name; }; ``` That attribute struct could be owned completely by RNA (not sure this is possible right now though). It could have attributes like `AttributeReader` and `AttributeWriter` from the C++ currently do.
Campbell Barton commented 2023-05-18 12:19:57 +02:00
Owner
Copy Link

@Baardaap thinking on this, most solutions have some reasonably large tradeoffs. Firstly, I'd be wary of making RNA handle ownership / allocations because it complicates something low level used quite widely, or, if allocations/ownership is extended, then it should be done so in a way that solves the problem of dangling pointers with Python more generally - not adding complexity for a handful of cases.

Similar to Hans suggestion, I think avoiding the direct pointer references to CustomDataLayer is the way to go, although it could be done without having RNA own the memory.

A way this problem could be handled with the current system could be to have a list of CustomDataLayer references (index or name), owned by the ID. So RNA would reference these, and adding/removing RNA layers could update them. Removing the layers would invalidate them in a way that also invalidates the Python object (see how ID has a void *py_instance member).

With this change:

  • Removing a layers-reference wouldn't reallocate other references.
  • Removing a layer-reference would invalidate the PyObject (if it exists) so future use would give a useful error message instead of accessing freed memory.

The down side is the table would need to be kept up to date and this isn't a generic solution - if we run into this elsewhere, it's going to require a similar solution to be implemented.

@Baardaap thinking on this, most solutions have some reasonably large tradeoffs. Firstly, I'd be wary of making RNA handle ownership / allocations because it complicates something low level used quite widely, or, if allocations/ownership is extended, then it should be done so in a way that solves the problem of dangling pointers with Python more generally - not adding complexity for a handful of cases. Similar to Hans suggestion, I think avoiding the direct pointer references to `CustomDataLayer` is the way to go, although it could be done without having RNA own the memory. A way this problem could be handled with the current system could be to have a list of CustomDataLayer references (index or name), owned by the ID. So RNA would reference these, and adding/removing RNA layers could update them. Removing the layers would invalidate them in a way that also invalidates the Python object (see how `ID` has a `void *py_instance` member). With this change: - Removing a layers-reference wouldn't reallocate other references. - Removing a layer-reference would invalidate the `PyObject` (if it exists) so future use would give a useful error message instead of accessing freed memory. The down side is the table would need to be kept up to date and this isn't a generic solution - if we run into this elsewhere, it's going to require a similar solution to be implemented.
Martijn Versteegh commented 2023-05-18 15:48:50 +02:00
Member
Copy Link

A way this problem could be handled with the current system could be to have a list of CustomDataLayer references (index or name), owned by the ID. So RNA would reference these, and adding/removing RNA layers could update them. Removing the layers would invalidate them in a way that also invalidates the Python object (see how ID has a void *py_instance member).

which is (more or less) what I'm implementing right now, and describing in my previous post, I think? I have the list of references owned by the CustomData though.

> > A way this problem could be handled with the current system could be to have a list of CustomDataLayer references (index or name), owned by the ID. So RNA would reference these, and adding/removing RNA layers could update them. Removing the layers would invalidate them in a way that also invalidates the Python object (see how `ID` has a `void *py_instance` member). which is (more or less) what I'm implementing right now, and describing in my previous post, I think? I have the list of references owned by the CustomData though.
Campbell Barton commented 2023-05-19 01:41:03 +02:00
Owner
Copy Link

@Baardaap yes, & storing in CustomData seems better (it's owned by the ID either way) although it would be good if we could avoid the bespoke iterators, it may be we can consider the API breakage acceptable for 4.0.

@Baardaap yes, & storing in `CustomData` seems better (it's owned by the `ID` either way) although it would be good if we could avoid the bespoke iterators, it may be we can consider the API breakage acceptable for 4.0.
Hans Goudey commented 2023-05-19 02:10:36 +02:00
Member
Copy Link

I would still like to push for a solution that doesn't push further complexity or a duplicate attribute list onto the rest of Blender. To me it does seem worth exploring adding the option to use a struct as a handle to Blender data. I'm sure there are other areas that were made more complex because that wasn't an option, and those cases will just keep coming up in the future.

But if RNA can't handle the complexity of owning a struct as a handle and we really do need a group of pointer-stable attributes, maintaining a duplicate list seems more like a short-term hack than a nice solution honestly. The right solution at that point is probably replacing CustomData with the equivalent of Vector<std::unique_ptr<Attribute>>.

I would still like to push for a solution that doesn't push further complexity or a duplicate attribute list onto the rest of Blender. To me it does seem worth exploring adding the option to use a struct as a handle to Blender data. I'm sure there are other areas that were made more complex because that wasn't an option, and those cases will just keep coming up in the future. But if RNA can't handle the complexity of owning a struct as a handle and we really do need a group of pointer-stable attributes, maintaining a duplicate list seems more like a short-term hack than a nice solution honestly. The right solution at that point is probably replacing `CustomData` with the equivalent of `Vector<std::unique_ptr<Attribute>>`.
Brecht Van Lommel commented 2023-05-19 14:07:05 +02:00
Owner
Copy Link

A solution that would work for 3.6 would be put a unique integer ID in CustomDataLayer, and then store that in PointerRNA.data. That's enough information to find the CustomDataLayer together with PointerRNA.owner_id.

Vector<std::unique_ptr<Attribute>> seems like the best long term solution.

A solution that would work for 3.6 would be put a unique integer ID in `CustomDataLayer`, and then store that in `PointerRNA.data`. That's enough information to find the `CustomDataLayer` together with `PointerRNA.owner_id`. `Vector<std::unique_ptr<Attribute>>` seems like the best long term solution.
Martijn Versteegh commented 2023-05-22 22:48:56 +02:00
Member
Copy Link

I'll try to fix up my attempt for a fix tomorrow. It might be a good starting point for further explorations and maybe a good stop-gap for 3.6 .

I agree with Hans that the extra list is ugly. Something like Brecht proposes would probably work, though it would mean searching all layers for each access, which is rather slow I guess.

Replacing the whole CustomData system for attributes with a smaller API more tailored to the attribute system would be nice imho, but that would be a lot of work guess.

I'll try to fix up my attempt for a fix tomorrow. It might be a good starting point for further explorations and maybe a good stop-gap for 3.6 . I agree with Hans that the extra list is ugly. Something like Brecht proposes would probably work, though it would mean searching all layers for each access, which is rather slow I guess. Replacing the whole CustomData system for attributes with a smaller API more tailored to the attribute system would be nice imho, but that would be a lot of work guess.
Martijn Versteegh commented 2023-05-25 21:32:11 +02:00
Member
Copy Link

A small status update:

Pfew. It's quite a hard nut to crack, this one. Especially as I knew next to nothing about how the whole rna system works.

I'm currently in the stage where my patch does not immediately crash blender (though it doesn't work yet either), so progress...

I hope to have some more time to spend on this tomorrow evening.

A small status update: Pfew. It's quite a hard nut to crack, this one. Especially as I knew next to nothing about how the whole rna system works. I'm currently in the stage where my patch does not immediately crash blender (though it doesn't work yet either), so progress... I hope to have some more time to spend on this tomorrow evening.
👍 1
Chris Blackbourn commented 2023-06-18 00:09:05 +02:00
Member
Copy Link

Hey @Baardaap , would it be okay if I look into this one too? We might be able to tag-team on it... Feel free to DM me..
(Same with #107416 )

Hey @Baardaap , would it be okay if I look into this one too? We might be able to tag-team on it... Feel free to DM me.. (Same with #107416 )
Martijn Versteegh commented 2023-06-18 10:02:58 +02:00
Member
Copy Link

Sure. If you have any good Ideas go ahead.

I'm a bit stuck on this one. actually. The indirection table is exteremely error prone to get right (since CustomData is copied around willy nilly in lots of places, so add pointer members in CustomData is rather a nightmare.

Feel free to ask anything .

Sure. If you have any good Ideas go ahead. I'm a bit stuck on this one. actually. The indirection table is exteremely error prone to get right (since CustomData is copied around willy nilly in lots of places, so add pointer members in CustomData is rather a nightmare. Feel free to ask anything .
Chris Blackbourn added the
Interest
UV Editing
 label 2023-07-01 03:18:47 +02:00
Hans Goudey commented 2023-09-07 21:16:34 +02:00
Member
Copy Link

In 4.0 this specific situation (editing UVs after normals) will be resolved by #108014 which moves the cached face corner normals out of CustomData. It's not a complete solution for every similar problem, but at least it's not caused by a cache which is much less predictable than an explicit action.

In 4.0 this specific situation (editing UVs after normals) will be resolved by #108014 which moves the cached face corner normals out of `CustomData`. It's not a complete solution for every similar problem, but at least it's not caused by a cache which is much less predictable than an explicit action.
Hans Goudey added
Type
Bug
 and removed
Type
Report
 labels 2023-09-11 16:53:41 +02:00
Dalai Felinto commented 2023-09-12 12:09:35 +02:00
Owner
Copy Link

Given that #108014 is no longer aimed at 4.0 what is the status of this task?

On a related note, what was agreed on regarding 3.6. Is this fixed there?

Is it possible to have a solution that addresses both 3.6 and 4.0 issues?

Given that #108014 is no longer aimed at 4.0 what is the status of this task? On a related note, what was agreed on regarding 3.6. Is this fixed there? Is it possible to have a solution that addresses both 3.6 and 4.0 issues?
Hans Goudey commented 2023-09-12 13:23:24 +02:00
Member
Copy Link

This is a large fundamental issue with the current CustomData design. The generic UV commit might have exposed the issue in this one case, but the issue with the CustomData Python API has existed for years, since the beginning.

Personally I don't think there's a good solution to this that isn't a hack. I think it's best to live with the issue (this case will still be solved in 4.1) until we get to a larger refactor of attribute storage to replace CustomData. It's probably worth writing down a preliminary design for that.

This is a large fundamental issue with the current `CustomData` design. The generic UV commit might have exposed the issue in this one case, but the issue with the `CustomData` Python API has existed for years, since the beginning. Personally I don't think there's a good solution to this that isn't a hack. I think it's best to live with the issue (this case will still be solved in 4.1) until we get to a larger refactor of attribute storage to replace `CustomData`. It's probably worth writing down a preliminary design for that.
Campbell Barton commented 2023-09-13 02:20:03 +02:00
Owner
Copy Link

Since this is an existing limitation with RNA access to CustomData (and would happen when moving any data to custom data), I agree that it's reasonable to accept the limitation.

Proposing to close this task and add a section to the Mesh API docs so script authors are aware of this.

Since this is an existing limitation with RNA access to CustomData (and would happen when moving _any_ data to custom data), I agree that it's reasonable to accept the limitation. Proposing to close this task and add a section to the Mesh API docs so script authors are aware of this.
👍 1
Andrej commented 2023-09-16 10:37:53 +02:00
Contributor
Copy Link

Proposing to close this task and add a section to the Mesh API docs so script authors are aware of this.

Met similar issue crashing Blender in #112432 with the code below.

It was pretty hard to debug and track down the issue down given that it occured not consistently but only sometimes, kind of randomly. And one of the worst thing about debugging it - crash occured only after python script was finished, so debugging it you're unsure on what line exactly it occurs.

(btw any idea what triggers the crash? tried to update the screen / update viewport in the process but it didn't triggered it, crash occured only after python script finished)

Is it possible to replace existing layers that become invalidated with invalid references instead of leading it to crashes? I mean in similar way it works with objects, meshes when they are replaced with something like <bpy_struct, Mesh invalid> and accessing it's parameters returns ReferenceError: StructRNA of type Object has been removed.

import bpy
from mathutils import Vector

mesh = bpy.context.object.data
uv_layer = mesh.uv_layers.active.data
vertex_colors = mesh.vertex_colors.new().data
    
for i in range(len(mesh.loops)):
    uv_layer[i].uv = Vector((1,1)).xy
print("finished")

And just to clearify - what would it be the correct way to create 2 vertex layer and access them? The one from example below?

# cannot use `layer1` from `.new()` since it'll be invalidated on the next line
layer_1 = mesh.vertex_colors.new()

layer_2 = mesh.vertex_colors.new().data
layer_1 = mesh.vertex_colors[0].data
> Proposing to close this task and add a section to the Mesh API docs so script authors are aware of this. Met similar issue crashing Blender in #112432 with the code below. It was pretty hard to debug and track down the issue down given that it occured not consistently but only sometimes, kind of randomly. And one of the worst thing about debugging it - crash occured only after python script was finished, so debugging it you're unsure on what line exactly it occurs. (btw any idea what triggers the crash? tried to update the screen / update viewport in the process but it didn't triggered it, crash occured only after python script finished) Is it possible to replace existing layers that become invalidated with invalid references instead of leading it to crashes? I mean in similar way it works with objects, meshes when they are replaced with something like `<bpy_struct, Mesh invalid>` and accessing it's parameters returns `ReferenceError: StructRNA of type Object has been removed`. ```python import bpy from mathutils import Vector mesh = bpy.context.object.data uv_layer = mesh.uv_layers.active.data vertex_colors = mesh.vertex_colors.new().data for i in range(len(mesh.loops)): uv_layer[i].uv = Vector((1,1)).xy print("finished") ``` And just to clearify - what would it be the correct way to create 2 vertex layer and access them? The one from example below? ```python # cannot use `layer1` from `.new()` since it'll be invalidated on the next line layer_1 = mesh.vertex_colors.new() layer_2 = mesh.vertex_colors.new().data layer_1 = mesh.vertex_colors[0].data ```
Thomas Dinges commented 2023-10-06 10:53:44 +02:00
Owner
Copy Link

@ideasman42 Any update here? If this is a limitation, please update the docs and close this task.

@ideasman42 Any update here? If this is a limitation, please update the docs and close this task.
Martijn Versteegh commented 2023-10-06 11:40:08 +02:00
Member
Copy Link

I'm a bit out of ideas to solve this cleanly. The only things I could think of were baroque constructions adding extra levels of indirection to the RNA pointers and none of my experiments came close to actually working. I think it's best to just document it. Though it is quite a nasty limitation.

We could maybe add some extra checks in the RNA pointer dereferencing to catch the crash and print a warning instead, though I'm not really sure if that would need equally baroque retrofitted machinery.

I'm a bit out of ideas to solve this cleanly. The only things I could think of were baroque constructions adding extra levels of indirection to the RNA pointers and none of my experiments came close to actually working. I think it's best to just document it. Though it is quite a nasty limitation. We could *maybe* add some extra checks in the RNA pointer dereferencing to catch the crash and print a warning instead, though I'm not really sure if that would need equally baroque retrofitted machinery.
Campbell Barton commented 2023-10-11 08:24:27 +02:00
Owner
Copy Link

Submitted a PR !113525 to resolve this, although it's more intrusive than I'd like.

Submitted a PR !113525 to resolve this, although it's more intrusive than I'd like.
Hans Goudey commented 2024-01-08 22:22:03 +01:00
Member
Copy Link

Going to take the liberty of labeling this a known issue, since there is no good quick solution. Better to work around the issue for now. That doesn't mean there won't be a solution, but just that it shouldn't be expected in the short term.

Going to take the liberty of labeling this a known issue, since there is no good quick solution. Better to work around the issue for now. That doesn't mean there won't be a solution, but just that it shouldn't be expected in the short term.
Hans Goudey added
Type
Known Issue
 and removed
Type
Bug
 labels 2024-01-08 22:22:12 +01:00
Martijn Versteegh commented 2024-01-09 22:28:54 +01:00
Member
Copy Link

I agree.

I think the solutions are too convoluted, especially since we plan on changing the CustomData storage in the long run anyway.

I agree. I think the solutions are too convoluted, especially since we plan on changing the CustomData storage in the long run anyway.
Hans Goudey added
Priority
Normal
 and removed
Priority
High
 labels 2024-01-09 22:41:04 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
main
brush-assets-project
blender-v4.1-release
blender-v3.3-release
blender-v3.6-release
universal-scene-description
blender-v3.6-temp_wmoss_animrig_public
temp-sculpt-dyntopo
gpencil-next
anim/animation-id-113594
blender-v4.0-release
blender-projects-basics
bridge-curves
sculpt-blender
asset-browser-frontend-split
asset-shelf
tmp-usd-python-mtl
tmp-usd-3.6
blender-v3.5-release
blender-v2.93-release
realtime-clock
sculpt-dev
bevelv2
xr-dev
v4.1.1
v3.6.11
v3.3.18
v4.1.0
v3.3.17
v3.6.10
v3.6.9
v3.3.16
v3.6.8
v3.6.7
v3.3.14
v4.0.2
v4.0.1
v4.0.0
v3.6.5
v3.3.12
v3.6.4
v3.6.3
v3.3.11
v3.6.2
v3.3.10
v3.6.1
v3.3.9
v3.6.0
v3.3.8
v3.3.7
v2.93.18
v3.5.1
v3.3.6
v2.93.17
v3.5.0
v2.93.16
v3.3.5
v3.3.4
v2.93.15
v2.93.14
v3.3.3
v2.93.13
v2.93.12
v3.4.1
v3.3.2
v3.4.0
v3.3.1
v2.93.11
v3.3.0
v3.2.2
v2.93.10
v3.2.1
v3.2.0
v2.83.20
v2.93.9
v3.1.2
v3.1.1
v3.1.0
v2.83.19
v2.93.8
v3.0.1
v2.93.7
v3.0.0
v2.93.6
v2.93.5
v2.83.18
v2.93.4
v2.93.3
v2.83.17
v2.93.2
v2.93.1
v2.83.16
v2.93.0
v2.83.15
v2.83.14
v2.83.13
v2.92.0
v2.83.12
v2.91.2
v2.83.10
v2.91.0
v2.83.9
v2.83.8
v2.83.7
v2.90.1
v2.83.6.1
v2.83.6
v2.90.0
v2.83.5
v2.83.4
v2.83.3
v2.83.2
v2.83.1
v2.83
v2.82a
v2.82
v2.81a
v2.81
v2.80
v2.80-rc3
v2.80-rc2
v2.80-rc1
v2.79b
v2.79a
v2.79
v2.79-rc2
v2.79-rc1
v2.78c
v2.78b
v2.78a
v2.78
v2.78-rc2
v2.78-rc1
v2.77a
v2.77
v2.77-rc2
v2.77-rc1
v2.76b
v2.76a
v2.76
v2.76-rc3
v2.76-rc2
v2.76-rc1
v2.75a
v2.75
v2.75-rc2
v2.75-rc1
v2.74
v2.74-rc4
v2.74-rc3
v2.74-rc2
v2.74-rc1
v2.73a
v2.73
v2.73-rc1
v2.72b
2.72b
v2.72a
v2.72
v2.72-rc1
v2.71
v2.71-rc2
v2.71-rc1
v2.70a
v2.70
v2.70-rc2
v2.70-rc
v2.69
v2.68a
v2.68
v2.67b
v2.67a
v2.67
v2.66a
v2.66
v2.65a
v2.65
v2.64a
v2.64
v2.63a
v2.63
v2.61
v2.60a
v2.60
v2.59
v2.58a
v2.58
v2.57b
v2.57a
v2.57
v2.56a
v2.56
v2.55
v2.54
v2.53
v2.52
v2.51
v2.50
v2.49b
v2.49a
v2.49
v2.48a
v2.48
v2.47
v2.46
v2.45
v2.44
v2.43
v2.42a
v2.42
v2.41
v2.40
v2.37a
v2.37
v2.36
v2.35a
v2.35
v2.34
v2.33a
v2.33
v2.32
v2.31a
v2.31
v2.30
v2.28c
v2.28a
v2.28
v2.27
v2.26
v2.25
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
Martijn Versteegh
13 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#107500
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?