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

Bastien Montagne commented

2023-08-15 20:16:43 +02:00

Owner

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

labels 2023-08-15 20:16:43 +02:00

Bastien Montagne commented

2023-08-15 20:17:10 +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

Bastien Montagne commented

2023-08-15 20:19:04 +02:00

Author

Owner

@blender-bot build

@blender-bot build

Bastien Montagne requested review from Brecht Van Lommel 2023-08-15 20:19:58 +02:00

Bastien Montagne commented

2023-08-15 20:22:26 +02:00

Author

Owner

@blender-bot build

@blender-bot build

Bastien Montagne commented

2023-08-16 09:57:21 +02:00

Author

Owner

@blender-bot build windows

@blender-bot build windows

Bastien Montagne commented

2023-08-16 10:47:40 +02:00

Author

Owner

@blender-bot build windows

@blender-bot build windows

Brecht Van Lommel commented

2023-08-16 10:48:38 +02:00

Owner

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?

Bastien Montagne commented

2023-08-16 11:12:37 +02:00

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.

Bastien Montagne commented

2023-08-16 11:27:21 +02:00

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 referenced this issue from a commit

2023-08-16 16:22:07 +02:00

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

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

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

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.