Page MenuHome

Fix T98293: Scene stats info not updated after new OBJ import
ClosedPublic

Authored by Aras Pranckevicius (aras_p) on May 23 2022, 11:18 AM.

Details

Summary

The importer was not doing a notification that the scene has changed, so the bottom status bar scene stats info was not updated right after the new OBJ import. Fixes T98293.

Diff Detail

Repository
rB Blender

Event Timeline

Aras Pranckevicius (aras_p) requested review of this revision.May 23 2022, 11:18 AM
Aras Pranckevicius (aras_p) created this revision.
Iyad Ahmed (iyadahmed2001) resigned from this revision.EditedMay 23 2022, 11:41 AM

Not sure if this or WM_main_add_notifier(NC_SCENE | ND_FRAME, data->scene); is more correct too,
leaving it to someone else

I would instead use: WM_event_add_notifier(C, NC_SCENE | ND_LAYER_CONTENT, scene); which is what we use for the OBJECT_OT_add operator.

Iyad Ahmed (iyadahmed2001) added inline comments.
source/blender/io/wavefront_obj/importer/obj_importer.cc
94 ↗(On Diff #51737)

Based on Dalai's answer this should be changed to
WM_event_add_notifier(C, NC_SCENE | ND_LAYER_CONTENT, scene);

we can check if it works too

Iyad Ahmed (iyadahmed2001) requested changes to this revision.May 23 2022, 12:56 PM
This revision now requires changes to proceed.May 23 2022, 12:56 PM
Julian Eisel (Severin) requested changes to this revision.EditedMay 23 2022, 1:05 PM

NC_OBJECT | ND_TRANSFORM should be sent when an existing object was transformed, and NC_SCENE | ND_FRAME when the current frame or frame data changed. So both are rather arbitrary choices (although they will probably trigger some of the necessary redraws/refreshes).

We need to notify that the visible contents of the scene or more precisely of the view-layer changed. But in addition, selection and the active object were changed, so we should notify about this too. Best copy one of the existing functions adding objects, e.g. object_add_named_exec() does this:

WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene);
WM_event_add_notifier(C, NC_SCENE | ND_OB_ACTIVE, scene);
WM_event_add_notifier(C, NC_SCENE | ND_LAYER_CONTENT, scene);
ED_outliner_select_sync_from_object_tag(C);

Lastly, I'm not sure what the best location for this is. If the import happens through the other importer_main(), the notifiers will be missing. Should that function be used for anything else than the tests, that would be an issue.
I think it's reasonable to say that these functions only deal with the scene data, while the UI should be handled by the operator, or whatever calls this. We do this in other places too. That means the snipped above is added to wm_obj_import_exec(). The OBJ functions should document that they change selection, the active object and that thus the caller should trigger UI updates if needed.


So my suggestion:

  • Paste snippet above into wm_obj_import_exec()
  • Note in the function comment of at least OBJ_import() that besides adding objects, this changes selection and the active object, and thus the UI needs to be updated accordingly.

static_cast<void>(CTX_data_ensure_evaluated_depsgraph(C));
@Aras Pranckevicius (aras_p) Since your update after the initial commit added DEG_id_tag_update calls etc, please see if removing the above line causes any problem. I had written it to force a viewport refresh. I have been told that it's not the right way.

Aras Pranckevicius (aras_p) edited the summary of this revision. (Show Details)
  • Moved UI update bits into more proper place, per Julian's suggestion.
  • Removed CTX_data_ensure_evaluated_depsgraph call that is not really needed (none of the other importers call it either), as per Ankit's comment.

static_cast<void>(CTX_data_ensure_evaluated_depsgraph(C));
@Aras Pranckevicius (aras_p) Since your update after the initial commit added DEG_id_tag_update calls etc, please see if removing the above line causes any problem. I had written it to force a viewport refresh. I have been told that it's not the right way.

This isn't really part of this bug fix, just a separate thing I noticed when checking this code. That's why I brought it up in the chat, not here ;) So I'd suggest to commit that removal separately.

This revision was not accepted when it landed; it landed in state Needs Review.May 23 2022, 7:44 PM
This revision was automatically updated to reflect the committed changes.