Readfile: Refactor several parts of the process #108016
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#108016
Loading…
Reference in New Issue
No description provided.
Delete Branch "mont29/blender:PR-setupappdata-refactor"
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?
This commit affects:
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
itselfis 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):
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...
bit more easy to follow, at least way more documented in code.
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 codeis also removed.
-- (Mostly) setup_app_data code in actual file reading 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.
Readfile: Refactor several parts of the process.to Readfile: Refactor several parts of the processe45be13f9e
to63eb9591f4
63eb9591f4
toa36d24d719
a36d24d719
tof8ea6a1417
f8ea6a1417
to3ef76d1e31
@blender-bot build
3ef76d1e31
to25f96f5a55
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);
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.@ -217,0 +271,4 @@
continue;
}
for (Library *new_lib_iter = static_cast<Library *>(new_bmain->libraries.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 oldMain
first.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) {
Could return early when
old_wm == nullptr
to avoid indentation.@ -217,0 +280,4 @@
}
BKE_id_remapper_add(remapper, &old_lib_iter->id, &new_lib_iter->id);
}
there should be a break here too, btw.
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.
@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.
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.
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 theBKE_libblock_free_data(..)
and related calls are local towm_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 towm_close_and_free
with an explanation why in most caseswm_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.
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
).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.
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
...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.Thanks for the notes @ideasman42 .
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.
Agreed, this was one the the things I was the least happy with... Replaced it by a new
BlendReadFileWMSetupData
struct.Done.
Done.
@blender-bot build
@blender-bot build