Fix #109670: attribute shared between geometries even when edited #111514

Jacques Lucke · 2023-08-25T12:20:20+02:00

Jacques Lucke commented

2023-08-25 12:20:20 +02:00

The RNA attribute API did not make sure that attributes are unshared when the data is accessed. This is necessary to avoid accidentally changing data on other geometries.

Unfortunately, we currently can't detect if the access is read-only or if the attribute data is modified. Therefore, the data is always copied when it was shared. This makes it a little bit less efficient in some cases, but that should still be ok. The old behavior could by brought back by implementing #112150.

The RNA attribute API did not make sure that attributes are unshared when the data is accessed. This is necessary to avoid accidentally changing data on other geometries. Unfortunately, we currently can't detect if the access is read-only or if the attribute data is modified. Therefore, the data is always copied when it was shared. This makes it a little bit less efficient in some cases, but that should still be ok. The old behavior could by brought back by implementing #112150.

Jacques Lucke added 1 commit 2023-08-25 12:20:32 +02:00

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

c362cdd0c5 fix

Jacques Lucke commented

2023-08-25 12:21:20 +02:00

@blender-bot build

Jacques Lucke requested review from Brecht Van Lommel 2023-08-25 12:21:38 +02:00

Jacques Lucke requested review from Hans Goudey 2023-08-25 12:21:38 +02:00

Hans Goudey commented

2023-08-25 13:08:45 +02:00

It's too bad this will affect Cycles, but maybe we can move Cycles to use the C++ attribute API (not RNA) for 4.0.

Jacques Lucke added 1 commit 2023-08-31 12:44:44 +02:00

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

6bea2fadf6 Merge branch 'main' into unshare-attribute-on-rna-access

Jacques Lucke commented

2023-08-31 12:45:17 +02:00

@blender-bot build

Hans Goudey commented

2023-09-05 04:42:27 +02:00

I know this is a fix, but I'm a little hesitant to accept this before we have at least some idea of how const correctness and implicit sharing should work properly with the RNA API. Do you have any thoughts about that? I thought making the Python API look more like the AttributeReader and AttributeWriter classes would make sense. But I'm not sure if that sort of const correctness is typical in Python or if things are just expected to be more explicit.

I know this is a fix, but I'm a little hesitant to accept this before we have at least some idea of how const correctness and implicit sharing should work properly with the RNA API. Do you have any thoughts about that? I thought making the Python API look more like the `AttributeReader` and `AttributeWriter` classes would make sense. But I'm not sure if that sort of const correctness is typical in Python or if things are just expected to be more explicit.

Jacques Lucke commented

2023-09-05 14:50:49 +02:00

We'll probably have to add a way to get read-only access to attributes at some point (e.g. attribute.data_readonly). I don't think it's worth breaking the api over this. It's also less common for things to be readonly that could also be mutable in Python, so making read-only the special case probably makes sense from a Python API perspectice.

We'll probably have to add a way to get read-only access to attributes at some point (e.g. `attribute.data_readonly`). I don't think it's worth breaking the api over this. It's also less common for things to be readonly that could also be mutable in Python, so making read-only the special case probably makes sense from a Python API perspectice.

Hans Goudey approved these changes 2023-09-07 21:23:00 +02:00

Hans Goudey left a comment

I'd like to consider Brecht's review blocking here, but this is okay with me. Can you create a followup task for .data_readonly just so we have that written down somewhere and we can point to it?

I'll change Cycles to use the C++ API directly for attributes in after this lands, since I don't want that to get slower.

I'd like to consider Brecht's review blocking here, but this is okay with me. Can you create a followup task for `.data_readonly` just so we have that written down somewhere and we can point to it? I'll change Cycles to use the C++ API directly for attributes in after this lands, since I don't want that to get slower.

Jacques Lucke referenced this pull request

2023-09-08 16:34:03 +02:00

Attributes: add .data_readonly field to attributes in RNA #112150

Jacques Lucke commented

2023-09-08 16:34:16 +02:00

I created #112150 as a follow up task.

👍 1

Brecht Van Lommel approved these changes 2023-09-12 12:29:53 +02:00

Brecht Van Lommel left a comment

Not ideal but I don't have a better solution without significant API changes.

Jacques Lucke merged commit 34b4487844 into main

2023-09-12 13:03:10 +02:00

Jacques Lucke deleted branch unshare-attribute-on-rna-access

2023-09-12 13:03:12 +02:00

Jacques Lucke referenced this issue from a commit

2023-09-12 13:03:13 +02:00

Fix #109670: attribute shared between geometries even when edited

Jacob Merrill commented

2023-10-16 15:32:59 +02:00

First-time contributor

I had been using a mode]al operator to using mouse / keyboard / joysticks to interact with a running simulation

I can't interact with the sim in py at all now without using a second mesh / passing in data that way - breaking any sort of ability to duplicate a setup.

I had been using a mode]al operator to using mouse / keyboard / joysticks to interact with a running simulation I can't interact with the sim in py at all now without using a second mesh / passing in data that way - breaking any sort of ability to duplicate a setup.

Jacob Merrill referenced this pull request

2023-10-17 14:16:39 +02:00

Regression: Can no longer set evaluated data in python for attributes #113621

Sign in to join this conversation.

No reviewers