Refactor: Properly formalize special versioning done after most of readfile code. #111147

Merged
Bastien Montagne merged 1 commits from mont29/blender:tmp-do-version-after-all into main 2023-08-16 16:22:06 +02:00

In a few cases (IPO conversion, Proxy conversion, ...), versioning
implies creating or removing IDs, and/or needs access to the whole Main
data-base.

So far this was done ad-hoc by adding some code at the end of
setup_app_data.

This commit formalizes this process by adding a BLO call
(BLO_read_do_version_after_setup) that will encapsulate all such
complex versioning code.

NOTE: This commit does not address the existing issue that this
versioning code is never performmed when linking new data (outside of
the 'opening a blendfile' context). This topic would require its own
design task.

NOTE: This commit does not fix the few current evil cases of ID creation in
regular versioning code. This will be addressed separately.

In a few cases (IPO conversion, Proxy conversion, ...), versioning implies creating or removing IDs, and/or needs access to the whole Main data-base. So far this was done ad-hoc by adding some code at the end of `setup_app_data`. This commit formalizes this process by adding a BLO call (`BLO_read_do_version_after_setup`) that will encapsulate all such complex versioning code. NOTE: This commit does not address the existing issue that this versioning code is never performmed when linking new data (outside of the 'opening a blendfile' context). This topic would require its own design task. NOTE: This commit does not fix the few current evil cases of ID creation in regular versioning code. This will be addressed separately.
Bastien Montagne added the
Interest
BlendFile
Module
Core
labels 2023-08-15 20:16:43 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne force-pushed tmp-do-version-after-all from c43bbc1ab6 to 37b0d6deec 2023-08-15 20:18:48 +02:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Bastien Montagne requested review from Brecht Van Lommel 2023-08-15 20:19:58 +02:00
Author
Owner

@blender-bot build

@blender-bot build
Author
Owner

@blender-bot build windows

@blender-bot build windows
Author
Owner

@blender-bot build windows

@blender-bot build windows

I would expect creating datablocks to be done as part of regular versioning and not after, since otherwise doing further versioning on those datablocks is easy to get wrong and hard to understand. If anything I would love to get rid of before/after lib linking distinction, not add a third.

I don't know exactly what it is that makes creating datablocks during versioning problematic now, but maybe that can be solved instead?

I would expect creating datablocks to be done as part of regular versioning and not after, since otherwise doing further versioning on those datablocks is easy to get wrong and hard to understand. If anything I would love to get rid of before/after lib linking distinction, not add a third. I don't know exactly what it is that makes creating datablocks during versioning problematic now, but maybe that can be solved instead?
Author
Owner

@brecht creating IDs during versioning is by definition not allowed, since these newly created IDs will also go through versioning, even though they are already at current state of code. It would be like versioning twice the same data, which can have completely unknown effects, and is totally unmanageable over time (since some code that works fine initially can become catastrophically broken by a future version versioning). Not to mention that depending on name (and therefore ordering), newly added ID may even be processed by current versioning code that is adding it.

The other way around can also be true in some cases (IDs added at some version, even though using current 'add ID' code, could in some rare case still need some further versioning at a later version). But adding IDs is already a rare case in versioning, so this would be exceptional imho. And actually even more of a reason to keep versioning creating IDs into its own specific area, so that it's at least easier to spot and check, rather than having to find the needle in our haystack of 'regular' versioning code.

Not to mention that some code, like the liboverride one, needs access to the whole final Main data-base, not some by-library sliced-up version.

If anything I would love to get rid of before/after lib linking distinction, not add a third.

How would that be possible? By getting rid of the 'before lib_linking' versioning code? That may be doable, but would require a lot of very careful checks to ensure nothing gets broken by doing so... Would not volunteer for that, imho really not worth the effort and risk.

@brecht creating IDs during versioning is by definition not allowed, since these newly created IDs will also go through versioning, even though they are already at current state of code. It would be like versioning twice the same data, which can have completely unknown effects, and is totally unmanageable over time (since some code that works fine initially can become catastrophically broken by a future version versioning). Not to mention that depending on name (and therefore ordering), newly added ID may even be processed by current versioning code that is adding it. The other way around can also be true in some cases (IDs added at some version, even though using current 'add ID' code, could in some rare case still need some further versioning at a later version). But adding IDs is already a rare case in versioning, so this would be exceptional imho. And actually even more of a reason to keep versioning creating IDs into its own specific area, so that it's at least easier to spot and check, rather than having to find the needle in our haystack of 'regular' versioning code. Not to mention that some code, like the liboverride one, needs access to the whole final Main data-base, not some by-library sliced-up version. > If anything I would love to get rid of before/after lib linking distinction, not add a third. How would that be possible? By getting rid of the 'before lib_linking' versioning code? That may be doable, but would require a lot of very careful checks to ensure nothing gets broken by doing so... Would not volunteer for that, imho really not worth the effort and risk.
Author
Owner

Another reason why adding IDs in do_version (before liblink) is bad: Current readfile mapping does not know about these new ID pointers, and would remap them to NULL. Or you need absolute horror hacks like that: https://projects.blender.org/blender/blender/pulls/111135/files#issuecomment-999267

Another reason why adding IDs in do_version (before liblink) is bad: Current readfile mapping does not know about these new ID pointers, and would remap them to NULL. Or you need absolute horror hacks like that: https://projects.blender.org/blender/blender/pulls/111135/files#issuecomment-999267
Brecht Van Lommel approved these changes 2023-08-16 14:26:28 +02:00
Brecht Van Lommel left a comment
Owner

I think there's pros and cons to both approaches. Making versioning linear I think is better in terms of being able to understand and ensure results are correct, but would require quite a few changes to be able to create datablocks as if they were created with an older version. But since this is also only needed for a few cases so far and it could be a big change, the approach of this PR seems practical.

I think there's pros and cons to both approaches. Making versioning linear I think is better in terms of being able to understand and ensure results are correct, but would require quite a few changes to be able to create datablocks as if they were created with an older version. But since this is also only needed for a few cases so far and it could be a big change, the approach of this PR seems practical.
Bastien Montagne force-pushed tmp-do-version-after-all from 37b0d6deec to b912b80e85 2023-08-16 16:21:33 +02:00 Compare
Bastien Montagne merged commit 28305c2c73 into main 2023-08-16 16:22:06 +02:00
Bastien Montagne deleted branch tmp-do-version-after-all 2023-08-16 16:22:08 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
Asset Browser Project
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
2 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#111147
No description provided.