COW and RNA #56172

New Issue

Bastien Montagne · 2018-07-31T13:52:09+02:00

Bastien Montagne commented

2018-07-31 13:52:09 +02:00

Currently RNA only has access to original data, never evaluated one. We have some RNA functions (mainly accessors) that need runtime-only data (generated by depsgraph into COW IDs), like e.g. the bounding box for object.dimension.

Currently, this is broken, usually dimensions just get outdated (see #55769), but this can also lead to a crash with curves in some cases, when trying to compute bbox from runtime.curve_cache (see #56064) e.g.

I can think of several ways to address the problem:

Pass bContext to getters/setters RNA functions (as we already do e.g. with update callbacks, dynamic enum generators, etc.).

This would imply adding a new getters/setters func pointers in RNA (with extended signature) I guess, with e.g. a flag to indicate that that property needs bContext in its accessors. This would also imply some sort of mechanism to set 'active' context in RNA (using a static pointer maybe?), which would need to be set before accessing any of those 'context' properties.
*** Python binding could do that easily, but may require some annoying adjustments to CPP code?

Give access to active depsgraph.

E.g. storing depsgraph in a static pointer when calling DEG_make_active(), and adding a DEG_get_active() callback? That would still give wrong data (and potentially nasty crash/corruption?) in case one is working with data from non-active depsgraph.
*** Thinking about render engines especially here, since those get data from a specific depsgraph - though since they do not access original data at all, maybe that would still be perfectly fine?

Forbid any eval-dependent data access in depsgraph (i.e. not more object.dimensions e.g.).
Make all eval-dependent data access through functions instead of properties.
That would make dimension non-editable! That would change a lot UI too…

Copy back things like object bbox from evaluated to orig datablocks (i.e. extend what we already do with a few things like object transform data iirc).

Tried that btw, does not work in many cases (e.g. with curves), since their bboxes are not computed during evaluation... We could enforce computing bboxes at that step, but meh… really not an elegant solution.

Solution 1. is probably doable, but most likely a PITA, with lots of code to edit, and potential side effects.

I have a rather strong preference for 2. now, would be by far the simplest/nicest solution imho. Not sure at all that would be fine and sane regarding DEG design though? :/ Also not clear in my mind currently, whether we have one active depsgraph for whole Blender, or one per window?

Other solutions I do not really like currently…

Currently RNA only has access to original data, never evaluated one. We have some RNA functions (mainly accessors) that need runtime-only data (generated by depsgraph into COW IDs), like e.g. the bounding box for `object.dimension`. Currently, this is broken, usually dimensions just get outdated (see #55769), but this can also lead to a crash with curves in some cases, when trying to compute bbox from runtime.curve_cache (see #56064) e.g. I can think of several ways to address the problem: # Pass bContext to getters/setters RNA functions (as we already do e.g. with update callbacks, dynamic enum generators, etc.). **This would imply adding a new getters/setters func pointers in RNA (with extended signature) I guess, with e.g. a flag to indicate that that property needs bContext in its accessors.** This would also imply some sort of mechanism to set 'active' context in RNA (using a static pointer maybe?), which would need to be set before accessing any of those 'context' properties. *** Python binding could do that easily, but may require some annoying adjustments to CPP code? # Give access to active depsgraph. **E.g. storing depsgraph in a static pointer when calling `DEG_make_active()`, and adding a `DEG_get_active()` callback?** That would still give wrong data (and potentially nasty crash/corruption?) in case one is working with data from non-active depsgraph. *** Thinking about render engines especially here, since those get data from a specific depsgraph - though since they do not access original data at all, maybe that would still be perfectly fine? - Forbid any eval-dependent data access in depsgraph (i.e. not more object.dimensions e.g.). - Make all eval-dependent data access through functions instead of properties. **That would make dimension non-editable!** That would change a lot UI too… # Copy back things like object bbox from evaluated to orig datablocks (i.e. extend what we already do with a few things like object transform data iirc). **Tried that btw, does not work in many cases (e.g. with curves), since their bboxes are not computed during evaluation...** We could enforce computing bboxes at that step, but meh… really not an elegant solution. Solution 1. is probably doable, but most likely a PITA, with lots of code to edit, and potential side effects. I have a rather strong preference for 2. now, would be by far the simplest/nicest solution imho. Not sure at all that would be fine and sane regarding DEG design though? :/ Also not clear in my mind currently, whether we have one active depsgraph for whole Blender, or one per window? Other solutions I do not really like currently…

Bastien Montagne self-assigned this 2018-07-31 13:52:09 +02:00

Bastien Montagne commented

2018-07-31 13:52:09 +02:00

Added subscribers: @mont29, @Sergey, @ideasman42, @brecht

Bastien Montagne commented

2018-10-09 11:04:34 +02:00

Spent some time yesterday checking on that problem again, and updated/refined my analysis…

We really need to do something (will fix crash itself now, but that won’t be an actual problem solution at all!)

Spent some time yesterday checking on that problem again, and updated/refined my analysis… We really need to do something (will fix crash itself now, but that won’t be an actual problem solution at all!)

Brecht Van Lommel commented

2018-10-09 11:39:14 +02:00

There was a decision to have only one active depsgraph for editing purposes, all the other depsgraphs are for viewing / rendering. So that should help solve this problem?

We are already flushing back object matrices to the original, and I think we can do the same for dimensions. So I think we should go with 5). It's not clear to me why bounding box computation not happening during evaluation would be a problem. We can make it happen during evaluation in that case, which is better for multithreading anyway. If for some reason that doesn't work it should be possible to wrap the code in such a way that the flushing still happens when the bounding box is computed.

Option 1) seems like it would make the API less predictable to use, 2) makes writing scripts harder, 3) 4) are not good because we need editable dimensions.

There was a decision to have only one active depsgraph for editing purposes, all the other depsgraphs are for viewing / rendering. So that should help solve this problem? We are already flushing back object matrices to the original, and I think we can do the same for dimensions. So I think we should go with 5). It's not clear to me why bounding box computation not happening during evaluation would be a problem. We can make it happen during evaluation in that case, which is better for multithreading anyway. If for some reason that doesn't work it should be possible to wrap the code in such a way that the flushing still happens when the bounding box is computed. Option 1) seems like it would make the API less predictable to use, 2) makes writing scripts harder, 3) 4) are not good because we need editable dimensions.

Bastien Montagne commented

2018-10-09 17:02:33 +02:00

@brecht Solution 2) does not involve modifying scripts, it’s RNA accessors that need to be updated (getting active depsgraph and calling relevant functions with relevant evaluated data). See a quick-slap test in P794 (seems to be working, but we’d need @Sergey to know whether that is 'allowed' or not I guess ;) ).

I’d go to solution 5) only if we cannot make 2) to work (since it’s much less generic, and involves extra processing that may not be needed in most cases).

@brecht Solution 2) does not involve modifying scripts, it’s RNA accessors that need to be updated (getting active depsgraph and calling relevant functions with relevant evaluated data). See a quick-slap test in [P794](https://archive.blender.org/developer/P794.txt) (seems to be working, but we’d need @Sergey to know whether that is 'allowed' or not I guess ;) ). I’d go to solution 5) only if we cannot make 2) to work (since it’s much less generic, and involves extra processing that may not be needed in most cases).

Brecht Van Lommel commented

2018-10-09 17:14:19 +02:00

But we already do 5) for object matrices. To me it seems best to have a single consistent solution, what breaks if we just copy back the boundbox to the original here?

void BKE_object_eval_done(Depsgraph *depsgraph, Object *ob)
{
    ...
    if (DEG_is_active(depsgraph)) {
        Object *ob_orig = DEG_get_original_object(ob);
        copy_m4_m4(ob_orig->obmat, ob->obmat);
        ...
    }
}

Meshes, curves and metaballs already compute the bounding box on the evaluated object as far as I can tell.

But we already do 5) for object matrices. To me it seems best to have a single consistent solution, what breaks if we just copy back the boundbox to the original here? ``` void BKE_object_eval_done(Depsgraph *depsgraph, Object *ob) { ... if (DEG_is_active(depsgraph)) { Object *ob_orig = DEG_get_original_object(ob); copy_m4_m4(ob_orig->obmat, ob->obmat); ... } } ``` Meshes, curves and metaballs already compute the bounding box on the evaluated object as far as I can tell.

blender-admin commented

2018-10-24 17:18:02 +02:00

This issue was referenced by e66084268c

This issue was referenced by e66084268ccc08bfde4b384211f5a248a805036d

Bastien Montagne commented

2018-10-24 17:22:11 +02:00

Changed status from 'Open' to: 'Resolved'

Bastien Montagne closed this issue

2018-10-24 17:22:11 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

COW and RNA #56172

Pass bContext to getters/setters RNA functions (as we already do e.g. with update callbacks, dynamic enum generators, etc.).

Give access to active depsgraph.

Copy back things like object bbox from evaluated to orig datablocks (i.e. extend what we already do with a few things like object transform data iirc).