Implementation of new blendfile compatibility policy #110109
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110109
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mont29:F-109151"
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 PR makes the following changes:
Blender does not open any file which minfileversion is newer than its
current version.
FileBrowser shows an error message instead of the content of the
library file.
shown in the footer of the UI.
with default option to 'Save As' instead.
another file, and current one is newer, the 'Save' option is replaced
by a 'Save As' one.
Technically, the hard check on minfileversion is implemented as a new
util in readfile code,
blo_check_minversion
, which is called as partof the process to get a FileData (aka BlendHandle) from a file path.
The 'soft warning' about data loss due to forward compatibility
limitations is stored as a new flag in Main,
has_forward_compatibility_issues
.NOTE: Final commit to main (and backports to both LTSs) will happen as
several commits.
Implements #109151.
PS: Attached two blend files, artificially created from Blender 5.0, and Blender 4.50, for easier testing ;)
@dfelinto @brecht @Sergey @ideasman42 Not sure who wants (and has time) to review this?
@blender-bot package
Package build started. Download here when ready.
Just some naming suggestions to solve ambiguity.
Overall on a code side seems fine, but would we'll need to check the interface and wording part of it.
@ -44,0 +50,4 @@
* 8, or 12 if subversion is provided. \param file_subversion the file subversion, if given value <
* 0, it is ignored, and only the `file_version` is used. */
void BKE_blender_version_blendfile_string_from_values(char *str_buff,
const size_t str_buff_len,
Is it the size of the buffer which needs to be 8, or the length (which makes it a requirement of the buffer size of 9) ?
@ -1080,1 +1083,4 @@
/** Check if minversion of the file is older than current Blender, return false if it is not.
* Should only be called after #read_file_dna was successfuly executed. */
static bool blo_check_minversion(FileData *fd, ReportList *reports)
The "check" semantic of functions is hard to understand by themselves. Having a function which "check condition" and return false if the condition is true makes it even harder.
Not sure the
blo_
prefix helps here, sois_minversion_older_than_blender
will be very hard to misread when reading code later onif (is_minversion_older_than_blender(fd, reports)) { ... }
Thanks, addressed.
Definitely.
@blender-bot package
Package build started. Download here when ready.
@ -458,6 +465,10 @@ int set_listbasepointers(struct Main *main, struct ListBase *lb[]);
((main)->versionfile < (ver) || \
((main)->versionfile == (ver) && (main)->subversionfile < (subver)))
#define MAIN_VERSION_OLDER_OR_EQUAL(main, ver, subver) \
MAIN_VERSION_OLDER_OR_EQUAL -> MAIN_VERSION_FILE_OLDER_OR_EQUAL for clarity?
My first guess without looking at the implementation was that this was not about the file version.
This follows naming of existing
MAIN_VERSION_ATLEAST
andMAIN_VERSION_OLDER
.If we want to state these are for file version, all need to be cleanedup first (which I don't mind doing at all, actually ;) ).
@ -128,0 +138,4 @@
"%d.%d.%d",
file_version_major,
file_version_minor,
file_subversion);
This displays the subversion as a patch version, but they are not the same.
Not sure what the best way to show it is, we can invent whatever syntax is different enough.
Replaced the
.
by adash
for the last (subversion) number.A dash is still pretty a relative subtle difference. Since it's only for development versions, perhaps something more verbose like this?
Was trying to not be too verbose, but sure, let's go for it then :)
@ -128,0 +141,4 @@
file_subversion);
}
else {
BLI_snprintf(str_buff, str_buff_len, "%d.%d", file_version_major, file_version_minor);
The subversion will almost always be non-zero, certainly for official releases. Just show it always?
Actually not, because in most cases (when major + minor versions are different), code passes the 'ignore'
-1
value for subversions, to avoid cluttering message to user with info that is not really exposed to them anywhere else 9since they typically never see the file subversion info anymore).So file subversion will typically only be shown when using builds from in-development main/release branches, when opening a file from a later build of the same current release (and file subversion has changed in-between).
@ -1080,1 +1083,4 @@
/** Check if minversion of the file is older than current Blender, return false if it is not.
* Should only be called after #read_file_dna was successfuly executed. */
static bool is_minversion_older_than_blender(FileData *fd, ReportList *reports)
I guess it's strictly older or equal. Could be named
is_minversion_newer_than_blender
and return value flipped if we want to brief.The name is actually fine (it does a strictly 'older than' check), but returned values were reversed compared to documentation and expected logical behavior. Thanks for catching this!
@ -1081,0 +1118,4 @@
MEM_freeN(fg);
return false;
}
MEM_freeN(fg);
Break to avoid going through the entire file? Or can this exist multiple times?
Indeed
@ -1255,0 +1323,4 @@
}
BKE_reports_clear(&sfile->runtime->reports);
if (!BKE_blendfile_is_readable(blendfile_path, &sfile->runtime->reports)) {
Is this opening the file on every redraw? That seems like it could be a performance problem, especially with network drivers where there might be significant latency.
Can this be cached, like I assume other file data show in the file browser is?
Eeeeek, good catch!
I'm fine with this now besides the one comment.
Maybe @dfelinto or @pablovazquez want to check if the UI matches their suggestions from the design task.
@blender-bot package
Package build started. Download here when ready.
Text:
Note that I think "sub" should never really be exposed to users. But this error case (incompatibility between the same release without a major version bump) will never happen anyways outside development versions so it is fine.
And I would stick to the shorter original proposed message or a variation of it: "File saved by newer Blender (4.0), expect loss of data!".
Shortened the message, note that putting the full blendfile path here is done for all reports by default, since ages (already there in commit from 2016). So changing this should not be part of this PR imho.
@blender-bot package
Package build started. Download here when ready.
We need to remove the exclamation point, since it conflicts with the
.
:Sub is still showing, even though I'm not on Blender 5.0:
When closing Blender I think we should say "could cause loss of data" (instead of may). Which would explain why there is no "Save" or overwrite option:
Indeed, will fix.
Well yes, it's still the same issue, this specific test file says it was saved with Blender 5 sub 7, and its minversion is 5 sub 3, so subversion is needed here.
Again, in real files this should never happen, since we do not expect ever a 5.0 sub x file to also have a 5.0 sub y minversion.
afaik 'could' and 'may' have the same meaning here?
@blender-bot package
Package build started. Download here when ready.
WIP: Initial implementation of new blendfile compatibility policy.to Implementation of new blendfile compatibility policy.Mostly small things, generally looking fine. One potentially bigger change, but I'm not convinced it's worth it, see inline comment.
This doesn't seem to be implemented? I don't see code to handle this.
@ -29,3 +29,3 @@
# Stats & Info
row.label(text=context.screen.statusbar_info(), translate=False)
layout.template_status_info()
Rather than just moving this all to an ad-hoc template (which kind of defeats the purpose of templates), I'd propose something like this:
I would rather not. I do think the whole status info area does deserve a template - this warning about forward compatibility is only the initial complex UI change here, there are potentially many more (missing linked data? missing external files? etc.). not to mention also the rest of the status info could use some love.
Find it way more logical and coherent as a single template
@ -51,0 +51,4 @@
/**
* Check whether a given path is actually a readable .blend file.
*/
bool BKE_blendfile_is_readable(const char *path, struct ReportList *reports);
The name & comment bother me a bit. This looks like a simple query, maybe checking if the path exists. But it actually opens the file and collects reports from that. So it's more of a
BKE_blendfile_attempt_to_open_and_close()
. A bit of an awkward name though, at least the comment should be more clear.This may not actually be needed though (see below regarding library hint drawing).
@ -1081,0 +1087,4 @@
{
BLI_assert(fd->filesdna != nullptr);
for (BHead *bhead = blo_bhead_first(fd); bhead; bhead = blo_bhead_next(fd, bhead)) {
if (bhead->code == BLO_CODE_GLOB) {
Yikes @ this big if-block - makes me scroll back and forth to check the scope :) Could use
continue
here?@ -6510,0 +6533,4 @@
uiBut *but;
const uiStyle *style = UI_style_get();
uiLayout *ui_abs = uiLayoutAbsolute(layout, false);
Yikes... But I see that's what regular reports do too... :/
@ -6510,0 +6538,4 @@
eUIEmbossType previous_emboss = UI_block_emboss_get(block);
UI_fontstyle_set(&style->widgetlabel);
int width = static_cast<int>(
Use functional style cast (
int width = int(...)
) https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast@ -6510,0 +6540,4 @@
UI_fontstyle_set(&style->widgetlabel);
int width = static_cast<int>(
BLF_width(style->widgetlabel.uifont_id, status_info_txt, strlen(status_info_txt)));
width = max_ii(width, static_cast<int>(10 * UI_SCALE_FAC));
Functional style cast.
@ -6510,0 +6597,4 @@
UI_BTYPE_BUT,
0,
ICON_ERROR,
static_cast<int>(3 * UI_SCALE_FAC),
Functional style cast.
@ -6510,0 +6617,4 @@
status_info_txt,
UI_UNIT_X,
0,
static_cast<short>(width + UI_UNIT_X),
Functional style cast.
@ -31,6 +31,7 @@
#include "DNA_userdef_types.h"
#include "DNA_workspace_types.h"
#include "BKE_blender_version.h"
Is this needed?
not anymore indeed
@ -1245,0 +1295,4 @@
const bool is_asset_browser = ED_fileselect_is_asset_browser(sfile);
const bool is_library_browser = !is_asset_browser &&
filelist_islibrary(sfile->files, blendfile_path, nullptr);
if (!is_asset_browser && !is_library_browser) {
Would remove this since the conditions are checked below anyway (redundant), and it has to be modified as new cases get added (which isn't clear while appending at the bottom of the function).
@ -1255,0 +1324,4 @@
if (!sfile->runtime->is_blendfile_status_set) {
BKE_reports_clear(&sfile->runtime->is_blendfile_readable_reports);
sfile->runtime->is_blendfile_readable = BKE_blendfile_is_readable(
This could all be part of the file-list loading:
BLO_blendhandle_from_file()
returns null infilelist_readjob_list_lib()
, aFL_HAS_INVALID_LIBRARY
file-list flag can be set.FileList
rather than being ignored (RPT_STORE
must be set!).is_library_browser
and theFL_HAS_INVALID_LIBRARY
flag here, and draw the hint with the reports in the file-list. (We won't draw a hint for recursive loading, even if the file-list has the "has invalid library" flag set, which seems like the wanted behavior.)BKE_blendfile_is_readable()
isn't needed then.I'm not feeling too strongly about this though. It seems sensible to do this way and avoids hacking this in with the runtime struct. But may be more complicated than worth it. I admit it's also nice to keep things together in this rather small scope.
i would rather keep that for a potential future improvement. Also keep in mind that this patch also needs to land in current LTS releases, so trying to keep it as limited as possible is important. It's already waaaaaayyy to big to my liking tbh :(
@ -1961,6 +1961,11 @@ BlendHandle *filelist_lib(FileList *filelist)
return filelist->libfiledata;
}
int filelist_files_num_entries(FileList *filelist)
Better make it clear in the name that this includes filtered out items. If I wanted to have the count of visible items, I'd just choose this function if it pops up in auto-completion (an actual bug).
I would expect such a function returning the amount of filtered entries to be called
filelist_files_num_entries_filtered
or something like that... This would also match the internal data (entries_num
vs.entries_filtered_num
).Will add comment, but imho name itself is fine.
@ -3764,3 +3977,1 @@
if (WM_operator_name_call(C, "WM_OT_save_mainfile", WM_OP_EXEC_DEFAULT, nullptr, nullptr) &
OPERATOR_CANCELLED)
{
if (bmain->has_forward_compatibility_issues) {
It's not clear why this case is different, better explain in a comment (why does it use invoke and why does it always execute the callback, even when the operator is cancelled?).
@ -3814,0 +4034,4 @@
UI_BTYPE_BUT,
0,
0,
has_forwardcompat_issues ? IFACE_("Save As...") : IFACE_("Save"),
Could also use a comment as to why
has_forwardcompat_issues
would change the displayed text.Implementation of new blendfile compatibility policy.to Implementation of new blendfile compatibility policyUI wise things look good, although the PR description could use some screenshots. Here are some.
On
Ctrl+S
with a file from a newer Blender version:Opening a new file while the current file from a newer Blender version contains unsaved changes:
Open a file supposedly saved in 5.0:
Attempting to link/append from a 5.0 file:
This was in initial implementation, but later on in that review it was decided to go back to a simple tooltip message for now.
Committed as the commits listed above, closing.
Pull request closed