Readfile: Refactor several parts of the process #108016

Merged
Bastien Montagne merged 14 commits from mont29/blender:PR-setupappdata-refactor into main 2023-06-05 13:54:54 +02:00

This commit affects:

  • Reading undo steps from memfile (aka 'Global Undo');
  • Handling of UI IDs (WindowManager, Workspaces and Screens) when
    opening a .blend file.

While no major changes are expected from a user PoV, there may be some
unexpected changes in rare edge-cases. None has been identified so far.

Undo step loading should be marginally faster (setup_app_data itself
is 2-3 times faster, as it does not do remapping anymore, which makes the
whole 'read undo step' process about 20% faster - but the most
time-consuming step on undo is the depsgraph processing, which remains
unchanged here).

This commit also solves some bugs (crashes) in some relatively uncommon
cases, like e.g. if the WM had an IDProperty pointing at an object and
UI is not loaded when opening a new .blend file with the 'Load UI' option
enabled (as in previous code on file opening WM ID would never be
remapped).

From a more technical side, this commit aims mainly at cleaning things
up, in preparation for the introduction of new 'no undo, no readfile'
type of handling (as part of the Brush Assets project):

  • Prevent WM code from doing (too much) horrible ID 'management' on
    its WM when opening a new file. It used to remove current WM from
    the Main database, store it in a temporary own list, and then free
    it itself...
  • Trying to make the complex logic behind WM handling on file reading a
    bit more easy to follow, at least way more documented in code.
  • Keep the handling of 'IDs being re-used from old Main' in a single
    place, as much as possible:
    -- Readfile code itself in undo case (because it's more efficient,
    and undo case is in a way simpler than actual .blend file
    reading case). The whole blo_lib_link_restore block of code
    is also removed.
    -- (Mostly) setup_app_data code in actual file reading case.
  • Sanitize the usage of the 'libmap' in readfile code in undo case
    (waaaaay too many pointers were added there, which was hiding some
    other issues in the related code, and potentially causing (in
    rare cases) memory addresses collisions.
This commit affects: * Reading undo steps from memfile (aka 'Global Undo'); * Handling of UI IDs (WindowManager, Workspaces and Screens) when opening a .blend file. While no major changes are expected from a user PoV, there may be some unexpected changes in rare edge-cases. None has been identified so far. Undo step loading should be marginally faster (`setup_app_data` itself is 2-3 times faster, as it does not do remapping anymore, which makes the whole 'read undo step' process about 20% faster - but the most time-consuming step on undo is the depsgraph processing, which remains unchanged here). This commit also solves some bugs (crashes) in some relatively uncommon cases, like e.g. if the WM had an IDProperty pointing at an object and UI is not loaded when opening a new .blend file with the 'Load UI' option enabled (as in previous code on file opening WM ID would never be remapped). From a more technical side, this commit aims mainly at cleaning things up, in preparation for the introduction of new 'no undo, no readfile' type of handling (as part of the Brush Assets project): - Prevent WM code from doing (too much) horrible ID 'management' on its WM when opening a new file. It used to remove current WM from the Main database, store it in a temporary own list, and then free it itself... - Trying to make the complex logic behind WM handling on file reading a bit more easy to follow, at least way more documented in code. - Keep the handling of 'IDs being re-used from old Main' in a single place, as much as possible: -- Readfile code itself in undo case (because it's more efficient, and undo case is in a way simpler than actual .blend file reading case). The whole `blo_lib_link_restore` block of code is also removed. -- (Mostly) setup_app_data code in actual file reading case. - Sanitize the usage of the 'libmap' in readfile code in undo case (waaaaay too many pointers were added there, which was hiding some other issues in the related code, and potentially causing (in rare cases) memory addresses collisions.
Hans Goudey changed title from Readfile: Refactor several parts of the process. to Readfile: Refactor several parts of the process 2023-05-17 18:05:45 +02:00
Bastien Montagne force-pushed PR-setupappdata-refactor from e45be13f9e to 63eb9591f4 2023-05-17 20:21:07 +02:00 Compare
Bastien Montagne force-pushed PR-setupappdata-refactor from 63eb9591f4 to a36d24d719 2023-05-19 10:52:49 +02:00 Compare
Bastien Montagne force-pushed PR-setupappdata-refactor from a36d24d719 to f8ea6a1417 2023-05-19 11:50:39 +02:00 Compare
Bastien Montagne force-pushed PR-setupappdata-refactor from f8ea6a1417 to 3ef76d1e31 2023-05-19 16:05:36 +02:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed PR-setupappdata-refactor from 3ef76d1e31 to 25f96f5a55 2023-05-19 17:41:11 +02:00 Compare
Bastien Montagne requested review from Brecht Van Lommel 2023-05-19 17:41:45 +02:00
Bastien Montagne requested review from Campbell Barton 2023-05-19 17:41:45 +02:00
Bastien Montagne requested review from Jacques Lucke 2023-05-19 17:41:46 +02:00
Jacques Lucke reviewed 2023-05-22 15:17:47 +02:00
Jacques Lucke left a comment
Member

Code generally looks reasonable but can't say I have a good intuition for what the code should be doing most of the time, so hard to say it's correct or wrong.

Code generally looks reasonable but can't say I have a good intuition for what the code should be doing most of the time, so hard to say it's correct or wrong.
@ -217,0 +260,4 @@
Main *old_bmain = reuse_data->old_bmain;
IDRemapper *remapper = reuse_data->remapper;
for (Library *old_lib_iter = static_cast<Library *>(old_bmain->libraries.first);
Member

We generally still use LISTBASE_FOREACH in C++ code, because writing for loops like you did here does not really improve readability or type safety.

We generally still use `LISTBASE_FOREACH` in C++ code, because writing for loops like you did here does not really improve readability or type safety.
mont29 marked this conversation as resolved
@ -217,0 +271,4 @@
continue;
}
for (Library *new_lib_iter = static_cast<Library *>(new_bmain->libraries.first);
Member

Would be good to avoid this kind of quadratic algorithms, even though the number of libraries is probably relatively low. To remove the quadratic run-time, just create a filepath -> Library * map for either the new or old Main first.

Would be good to avoid this kind of quadratic algorithms, even though the number of libraries is probably relatively low. To remove the quadratic run-time, just create a `filepath -> Library *` map for either the new or old `Main` first.
Author
Owner

I don't think that this added complexity will ever justify itself. At the very least, I would wait to get a real-life example where this change would give any measurable (and significant) benefit.

I don't think that this added complexity will ever justify itself. At the very least, I would wait to get a real-life example where this change would give any measurable (and significant) benefit.
@ -217,0 +400,4 @@
wmWindowManager *old_wm = static_cast<wmWindowManager *>(old_wm_list->first);
wmWindowManager *new_wm = static_cast<wmWindowManager *>(new_wm_list->first);
if (old_wm != nullptr) {
Member

Could return early when old_wm == nullptr to avoid indentation.

Could return early when `old_wm == nullptr` to avoid indentation.
mont29 marked this conversation as resolved
Bastien Montagne reviewed 2023-05-22 17:25:34 +02:00
@ -217,0 +280,4 @@
}
BKE_id_remapper_add(remapper, &old_lib_iter->id, &new_lib_iter->id);
}
Author
Owner

there should be a break here too, btw.

there should be a break here too, btw.
mont29 marked this conversation as resolved
Bastien Montagne added 2 commits 2023-05-22 17:47:23 +02:00
Bastien Montagne added 1 commit 2023-05-22 18:06:53 +02:00

I can't review this in any reasonable amount of time, it should have been broken up into smaller steps if you expect a good review. Like first moving code, renaming, then making logic changes in multiple steps.

I can't review this in any reasonable amount of time, it should have been broken up into smaller steps if you expect a good review. Like first moving code, renaming, then making logic changes in multiple steps.
Author
Owner

@brecht would have loved too, but this code is currently so entangled that it would not be practical to do so

@brecht would have loved too, but this code is currently so entangled that it would not be practical to do so

In my experience it is possible to break things up better than this, even for entangled code. The only way for me to review this would be to break it up into smaller steps myself, as I just can't understand the consequences of so many changes at once.

In my experience it is possible to break things up better than this, even for entangled code. The only way for me to review this would be to break it up into smaller steps myself, as I just can't understand the consequences of so many changes at once.
Author
Owner

I would be very interested in your estimation of required time (amount of work) to split this PR into smaller steps without breaking anything in current behavior of both file load and file undo... Again, current code (in main) has a lot of issues, which get hidden behind redundancy and over-processing in completely different parts of the code...

FYI I already extracted and committed separately a lot of cleanup and smaller fixes that could go in independantly.

I would be very interested in your estimation of required time (amount of work) to split this PR into smaller steps _without_ breaking anything in current behavior of both file load and file undo... Again, current code (in main) has a lot of issues, which get hidden behind redundancy and over-processing in completely different parts of the code... FYI I already extracted and committed separately a lot of cleanup and smaller fixes that could go in independantly.

I don't know how long it would take to split this up, a few hours? If I was implementing this I would have kept changes separate as I was working on it, to keep it clear for myself.

Anyway, I looked at this for 2 hours and I have only understood a small part of the impact. I guess you can just commit it without my review if you are confident this is the way to go.

I don't know how long it would take to split this up, a few hours? If I was implementing this I would have kept changes separate as I was working on it, to keep it clear for myself. Anyway, I looked at this for 2 hours and I have only understood a small part of the impact. I guess you can just commit it without my review if you are confident this is the way to go.
Brecht Van Lommel refused to review 2023-05-23 11:25:55 +02:00

Also finding review difficult, noticing some possible changes but they're more along the lines of minor refactoring, themselves somewhat opinionated.

Noting some issues issues although I don't consider them blocking:

  • wm_close_and_free_all used to be a central place where the WM and it's ID was freed, now additional calls that free the BKE_libblock_free_data(..) and related calls are local to wm_file_read_setup_wm_use_new which seems like low-level internals to be in-lining in a function that isn't primarily related to freeing the window manager. Suggest to have two functions, one that more explicitly clears the data, another that frees the ID (We often name these *_free_data & *_free elsewhere). This could be located next to wm_close_and_free with an explanation why in most cases wm_close_and_free should be used (maybe that name gets changed too).

  • Storing wmWindowManager::readfile_old_wm is awkward, if it were possible to pass this as an argument between functions that use it - instead of storing in the WM, as any mixup with flow control between when this is set and used will be confusing. Only if practical though.

  • Doc-strings for wm_file_read_setup_wm_init/wm_file_read_setup_wm_finalize should reference each other.

  • From what I can see the basic logic in setup_app_data is unchanged (no functional changes expected), although there has been some refactoring here which makes it difficult to tell. So I'll trust nothing broke with these changes (my own testing loading various old/new files seem to work as expected).

  • Typo's msatching, accross, versionning, remmaping.

Resigning from review, generally the patch seems fine but my notes are fairly surface level.

Also finding review difficult, noticing some possible changes but they're more along the lines of minor refactoring, themselves somewhat opinionated. Noting some issues issues although I don't consider them blocking: - `wm_close_and_free_all` used to be a central place where the WM and it's ID was freed, now additional calls that free the `BKE_libblock_free_data(..)` and related calls are local to `wm_file_read_setup_wm_use_new` which seems like low-level internals to be in-lining in a function that isn't primarily related to freeing the window manager. Suggest to have two functions, one that more explicitly clears the data, another that frees the ID (We often name these `*_free_data` & `*_free` elsewhere). This could be located next to `wm_close_and_free` with an explanation why in most cases `wm_close_and_free` should be used (maybe that name gets changed too). - Storing `wmWindowManager::readfile_old_wm` is awkward, if it were possible to pass this as an argument between functions that use it - instead of storing in the WM, as any mixup with flow control between when this is set and used will be confusing. Only if practical though. - Doc-strings for `wm_file_read_setup_wm_init/wm_file_read_setup_wm_finalize` should reference each other. - From what I can see the basic logic in `setup_app_data` is unchanged (no functional changes expected), although there has been some refactoring here which makes it difficult to tell. So I'll trust nothing broke with these changes (my own testing loading various old/new files seem to work as expected). - Typo's `msatching, accross, versionning, remmaping`. Resigning from review, generally the patch seems fine but my notes are fairly surface level.
Campbell Barton refused to review 2023-05-24 15:59:46 +02:00

From some extra testing loading all files in our lib directory, this patch causes a crash in proxy conversion (BKE_lib_override_library_main_proxy_convert).

 gdb --ex=r --args env ASAN_OPTIONS=check_initialization_order=0 ./blender ../lib/tests/libraries_and_linking/libraries/main_scene.blend

Crashes with a null pointer de-reference caused by scene->master_collection being NULL, attached stack-trace.


Note that

From some extra testing loading all files in our lib directory, this patch causes a crash in proxy conversion (`BKE_lib_override_library_main_proxy_convert`). ``` gdb --ex=r --args env ASAN_OPTIONS=check_initialization_order=0 ./blender ../lib/tests/libraries_and_linking/libraries/main_scene.blend ``` Crashes with a null pointer de-reference caused by `scene->master_collection` being NULL, attached stack-trace. ---- Note that
Author
Owner

From some extra testing loading all files in our lib directory, this patch causes a crash in proxy conversion (BKE_lib_override_library_main_proxy_convert).

 gdb --ex=r --args env ASAN_OPTIONS=check_initialization_order=0 ./blender ../lib/tests/libraries_and_linking/libraries/main_scene.blend

Crashes with a null pointer de-reference caused by scene->master_collection being NULL, attached stack-trace.


Note that

Ohhh nice catch... but this file is already completely freaking out main build actually, so don't think it's related to this patch.

> From some extra testing loading all files in our lib directory, this patch causes a crash in proxy conversion (`BKE_lib_override_library_main_proxy_convert`). > > ``` > gdb --ex=r --args env ASAN_OPTIONS=check_initialization_order=0 ./blender ../lib/tests/libraries_and_linking/libraries/main_scene.blend > ``` > > Crashes with a null pointer de-reference caused by `scene->master_collection` being NULL, attached stack-trace. > > ---- > > Note that Ohhh nice catch... but this file is already completely freaking out main build actually, so don't think it's related to this patch.
Member

Unlikely that I can add too much here if Campbell and Brecht can't fully review it. I'm fine with this being committed as, seems like we'll only know if there are more regressions by getting it into main...

Unlikely that I can add too much here if Campbell and Brecht can't fully review it. I'm fine with this being committed as, seems like we'll only know if there are more regressions by getting it into `main`...
Jacques Lucke refused to review 2023-05-25 19:02:31 +02:00

Ohhh nice catch... but this file is already completely freaking out main build actually, so don't think it's related to this patch.

The NULL pointer de-reference is caused by this patch (double checked that this doesn't happen in main), but that file does have other assertions in main - but not crashes from my testing.

> Ohhh nice catch... but this file is already completely freaking out main build actually, so don't think it's related to this patch. > The NULL pointer de-reference is caused by this patch (double checked that this doesn't happen in `main`), but that file does have other assertions in main - but not crashes from my testing.
Bastien Montagne added 3 commits 2023-05-26 19:43:00 +02:00
Bastien Montagne added 3 commits 2023-06-01 11:19:39 +02:00
Bastien Montagne added 2 commits 2023-06-01 16:36:31 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
3e40243bee
Remove ugly `old_wm` pointer in DNA WM struct, and use instead a temporary struct in WM readfile code.
Much cleaner solution, athough a bit more verbose as this still needs to
be shared between WM and setup_app_data code...
Author
Owner

Thanks for the notes @ideasman42 .

Noting some issues issues although I don't consider them blocking:

  • wm_close_and_free_all used to be a central place where the WM and it's ID was freed, now additional calls that free the BKE_libblock_free_data(..) and related calls are local to wm_file_read_setup_wm_use_new which seems like low-level internals to be in-lining in a function that isn't primarily related to freeing the window manager. Suggest to have two functions, one that more explicitly clears the data, another that frees the ID (We often name these *_free_data & *_free elsewhere). This could be located next to wm_close_and_free with an explanation why in most cases wm_close_and_free should be used (maybe that name gets changed too).

I think this is better actually, am not a big fan of these type-specific free functions, freeing should be handled through generic ID management usually. This case is a very special exception, since the freed ID is outside of any Main. So direct calls to lower-level API sounds OK to me. I would not 'make it easy to re-use' by adding some wrapper function around it, unless proven that other parts of the code absolutely need to do the same.

  • Storing wmWindowManager::readfile_old_wm is awkward, if it were possible to pass this as an argument between functions that use it - instead of storing in the WM, as any mixup with flow control between when this is set and used will be confusing. Only if practical though.

Agreed, this was one the the things I was the least happy with... Replaced it by a new BlendReadFileWMSetupData struct.

  • Doc-strings for wm_file_read_setup_wm_init/wm_file_read_setup_wm_finalize should reference each other.

Done.

  • Typo's msatching, accross, versionning, remmaping.

Done.

Thanks for the notes @ideasman42 . > Noting some issues issues although I don't consider them blocking: > > - `wm_close_and_free_all` used to be a central place where the WM and it's ID was freed, now additional calls that free the `BKE_libblock_free_data(..)` and related calls are local to `wm_file_read_setup_wm_use_new` which seems like low-level internals to be in-lining in a function that isn't primarily related to freeing the window manager. Suggest to have two functions, one that more explicitly clears the data, another that frees the ID (We often name these `*_free_data` & `*_free` elsewhere). This could be located next to `wm_close_and_free` with an explanation why in most cases `wm_close_and_free` should be used (maybe that name gets changed too). I think this is better actually, am not a big fan of these type-specific free functions, freeing should be handled through generic ID management usually. This case is a very special exception, since the freed ID is outside of any Main. So direct calls to lower-level API sounds OK to me. I would not 'make it easy to re-use' by adding some wrapper function around it, unless proven that other parts of the code absolutely need to do the same. > - Storing `wmWindowManager::readfile_old_wm` is awkward, if it were possible to pass this as an argument between functions that use it - instead of storing in the WM, as any mixup with flow control between when this is set and used will be confusing. Only if practical though. Agreed, this was one the the things I was the least happy with... Replaced it by a new `BlendReadFileWMSetupData` struct. > - Doc-strings for `wm_file_read_setup_wm_init/wm_file_read_setup_wm_finalize` should reference each other. Done. > - Typo's `msatching, accross, versionning, remmaping`. Done.
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne added 1 commit 2023-06-02 11:07:43 +02:00
Bastien Montagne added 1 commit 2023-06-05 12:38:45 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
d48ee5085b
Merge branch 'main' into PR-setupappdata-refactor
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne added 1 commit 2023-06-05 13:54:19 +02:00
Bastien Montagne merged commit ebb5643e59 into main 2023-06-05 13:54:54 +02:00
Bastien Montagne deleted branch PR-setupappdata-refactor 2023-06-05 13:54:55 +02:00
Sign in to join this conversation.
No reviewers
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
No Milestone
No project
No Assignees
4 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#108016
No description provided.