Page MenuHome

Blend Write: Add option for legacy mesh format
AbandonedPublic

Authored by Hans Goudey (HooglyBoogly) on Apr 7 2022, 6:15 AM.

Details

Summary

When changing the organization of custom data layers, backwards
compatibility is easy to address, but forward compatibility is more complex.
This commit adds an option to write files with a legacy mesh format to the
N-panel of the file browser in the "Save As" operator. The option will be used
first by D14077, and won't be committed before that.

It's only added to the "Save As" operator because it's possible to see the
operator properties before confirmation, unlike the regular file save operator.

Ref T95965

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 7 2022, 6:15 AM
Hans Goudey (HooglyBoogly) created this revision.
Campbell Barton (campbellbarton) requested changes to this revision.Jun 27 2022, 1:37 PM

Mostly this looks fine, I thought it could simplify the patch using write_flag P3031, but it introduces ambiguity for where the flag is expected to be used, so think this is fine as is.

Minor issues noted inline.

source/blender/windowmanager/intern/wm_files.c
3070–3072

The RNA_struct_property_is_set check doesn't make sense, removed for "copy", rB6a2c42a0d58e: Cleanup: remove redundant RNA_struct_property_is_set check

3208–3213

PROP_SKIP_SAVE should be set here.

This revision now requires changes to proceed.Jun 27 2022, 1:37 PM
  • Simplify retrieving operator property
  • Add back PROP_SKIP_SAVE
This revision is now accepted and ready to land.Jun 28 2022, 2:32 AM

What is the "failure mode" when a .blend file is saved NOT using the legacy mesh format, and is attempted to be opened in an older Blender version?

Having this as an option disabled by default is a forward compatibility breakage. Such things are always causing mess in a studio environments. If we have code to write mesh in a compatible way without interfering with artists at all, why to even think of user-level breaking changes?

If there is a very good reason (other than simplicity of code ;) the incompatible files are to be saved with a proper BLENDER_FILE_MIN_VERSION.

What is the "failure mode" when a .blend file is saved NOT using the legacy mesh format, and is attempted to be opened in an older Blender version?

It depends on the change. For D14685 it would be losing the "hidden" information of meshes, for D14077 it would be losing the bevel weights.
In the future as this refactor finishes up, it might be worse, since there would be no MVert layer saved in files.

Having this as an option disabled by default is a forward compatibility breakage. Such things are always causing mess in a studio environments.

That's right, it is, so the changes should be communicated well and the "min file version" should be set appropriately. I suppose conditionally based on whether this option is enabled, if you think it's doable and worth it.
To me breaking forward compatibility with an option of making it work doesn't seem so bad given the above.

If we have code to write mesh in a compatible way without interfering with artists at all, why to even think of user-level breaking changes?

As mentioned in T95965, the legacy mesh format cannot represent situations like "no material index layer" with the same semantics, so there has to be an explicit difference.
It also helps to keep the "legacy" areas in a specific place so the rest of the code can be improved. If we kept writing to files with this legacy format forever, we would never see the benefits in the file reading/writing area.

If there is a very good reason (other than simplicity of code ;) the incompatible files are to be saved with a proper BLENDER_FILE_MIN_VERSION.

I'm not sure I understand what you're suggesting here. I had assumed the patch actually making the mesh format changes would set the file min version, I usually do that sort of change closer to committing though, to avoid conflicts.

To me breaking forward compatibility with an option of making it work doesn't seem so bad given the above.

That is not how we prefer to work though. There must always be a very good reason for breaking changes. If the option is introduces it should be working in a way that users expects Blender to work. In this case the expectation always was that Blender preserves as much compatibility as possible.

So far here I am not convinced the proposed solution is the way to go. We can not be breaking forward compatibility so badly throughout a Blender series. Typically such changes are accumulated and the most breaking change is done as part of a beginning of a new series. With the explicit information given in the description it sounds that the more fitting to the spirit way would be avoid the option and always consider it is implicitly enabled, and remove the related code in Blender 4.0.

Which leads to the following point: there is no information about what happens if the Legacy Mesh Format option is enabled and the .blend file is read by the same Blender version. Is this option something lossy or is it designed to be lossless?

Btw, another possibility is to accumulate all such breaking changes and apply them all together at a pre-announced major version release, without going into hassles of the legacy format option. This also gives time to apply all required changes throughout the previous series to ensure Blender will not crash when opening newer .blend files.

If we kept writing to files with this legacy format forever, we would never see the benefits in the file reading/writing area.

For reading you will see an immediate benefit. For writing you will see benefit with the next major Blender version.

I'm not sure I understand what you're suggesting here. I had assumed the patch actually making the mesh format changes would set the file min version.

What i was suggesting is what you wrote above: write files with actual BLENDER_FILE_MIN_VERSION which is needed to read them. At a very least if the legacy option is not used the file needs to be written with a higher BLENDER_FILE_MIN_VERSION. I am not fully sure what happens when a file with higher min version is opened. If Blender still opens the file (and not just erorrs out that the file is too new) then we can always store the new min version without writing it conditionally.

I usually do that sort of change closer to committing though, to avoid conflicts.

This is bad communication then, which also kinda defeating purpose of the code review. How can a reviewer know that the change is not included into the patch because author remembers it but wants to hold off? How can a reviewer know that the change will not be forgotten when the patch is actually committed?

Patch should be matching what will go into git.

Not saying the BLENDER_FILE_MIN_VERSION should be changed in this patch as it is not really affecting what goes into the .blend file. But the way you phrased the sentence sounds like it goes agains the "patch matches commit" philosophy which is not good.

We can not be breaking forward compatibility so badly throughout a Blender series. Typically such changes are accumulated and the most breaking change is done as part of a beginning of a new series.

Okay, this seems like a pretty solid "no" for this patch then. I'm a bit surprised there isn't a stronger consensus around this, if this is actually the way we do things, but it is what it is.

there is no information about what happens if the Legacy Mesh Format option is enabled and the .blend file is read by the same Blender version. Is this option something lossy or is it designed to be lossless?

In its current form in this and the other patches, it's probably lossy.

Btw, another possibility is to accumulate all such breaking changes and apply them all together at a pre-announced major version release, without going into hassles of the legacy format option.

I don't want to wait a year to finish this mesh data structure refactor I'm working on, but I am okay with accumulating the legacy mesh format writing changes and removing them in the future.

For reading you will see an immediate benefit. For writing you will see benefit with the next major Blender version.

If we always write with the legacy mesh format, there will be no improvement for reading or writing (it will get worse/slower actually) until 4.0, or whenever we stop writing in the old mesh format.

Okay, this seems like a pretty solid "no" for this patch then.

Not sure how you came from "typically is not how we deal with things" to "solid no".
If something is really in the way of development and there is no way moving forward without breaking things in a manner which we try not to, then there are still ways to schedule work and breaking changes in a nice and manageable way.

For someone who just stumbled upon this change there are many questions mainly caused by the fact that something what has big implications on users was accepted so easily without stating those implications.

In its current form in this and the other patches, it's probably lossy.

Here comes another concern: enabling an option by accident will lead to a data loss without any warning.

I don't want to wait a year to finish this mesh data structure refactor I'm working on, but I am okay with accumulating the legacy mesh format writing changes and removing them in the future.

Similarly, I do not want forward compatibility being taken so easily. It always have serious implications in studio environments, especially here. Such things needs to be better way planned and communicated.

Not sure how you came from "typically is not how we deal with things" to "solid no".

Fair enough. I'm just trying to find a path forward that makes people happy and doesn't cause more problems in the future.
It feels simpler if everyone has a "yes or no" opinion but that isn't always the case ;)
The goal was to avoid making things too complicated, and to benefit from the simplicity coming from the refactors.
The points you're raising make the "not too complicated" argument much less obvious.

Similarly, I do not want forward compatibility being taken so easily. It always have serious implications in studio environments, especially here. Such things needs to be better way planned and communicated.

I don't want to take breaking forward compatibility lightly, but I think we should also be realistic and not try to maintain these forward compatibility changes forever.
In that case, using the legacy format until a large release (4.0) seems like a reasonable compromise to me.
That would give us plenty of time to communicate the change, and would allow time to finish the rest of the refactors from T95965 so all the compatibility changes happen together.
What do you think about that approach?

To me it sounds perfect!

Okay, I'll do that!