Refactor: Blend file reading asserts for broken pointers #120997

Brecht Van Lommel · 2024-04-23T21:30:13+02:00

Brecht Van Lommel commented

2024-04-23 21:30:13 +02:00

Previously these were silently ignored. In general they should not happen,
but due to versioning and bugs some blend files do have broken pointers.

The known cases are explicitly marked as such now, and the assert is skipped.
Those cases must correctly handle the pointer being null.

Future Blender versions are likely to write more invalid pointers, which
would then trigger these asserts in older versions. I guess as long as
it is only an assert this is ok, but could be annoying in some cases.
Though with these asserts they are more likely to be caught and fixed.

Previously these were silently ignored. In general they should not happen, but due to versioning and bugs some blend files do have broken pointers. The known cases are explicitly marked as such now, and the assert is skipped. Those cases must correctly handle the pointer being null. Future Blender versions are likely to write more invalid pointers, which would then trigger these asserts in older versions. I guess as long as it is only an assert this is ok, but could be annoying in some cases. Though with these asserts they are more likely to be caught and fixed.

Brecht Van Lommel added 2 commits 2024-04-23 21:30:23 +02:00

9262110ce6 Refactor: Use typed functions for blend file data reading

This makes the read and write API functions match more closely, and adds
asserts to check that the data size is as expected.

There are still a few places remaining that use BLO_read_data_address
and similar, these should eventually be replaced as well.

59ca145ef9 Refactor: Blend file reading asserts for broken pointers

Previously these were silently ignored. In general they should not happen,
but due to versioning and bugs some blend files do have broken pointers.

The known cases are explicitly marked as such now, and the assert is skipped.
Those cases must correctly handle the pointer being null.

Future Blender versions are likely to write more invalid pointers, which
would then trigger these asserts in older versions. I guess as long as
it is only an assert this is ok, but could be annoying in some cases.
Though with these asserts they are more likely to be caught and fixed.

Brecht Van Lommel added a new dependency 2024-04-23 21:37:27 +02:00

#120996 Remesh Voxel Size cannot be interactively set larger than Scene Unit Scale

Brecht Van Lommel removed a dependency 2024-04-23 21:37:33 +02:00

#120996 Remesh Voxel Size cannot be interactively set larger than Scene Unit Scale

Brecht Van Lommel added a new dependency 2024-04-23 21:38:26 +02:00

#120994 Refactor: Use typed functions for blend file data reading

Brecht Van Lommel force-pushed refactor-assert-broken-pointer from 59ca145ef9 to 1cf8a76b9a

2024-04-24 17:07:57 +02:00

Compare

Brecht Van Lommel commented

2024-04-24 17:09:36 +02:00

@blender-bot build

Brecht Van Lommel requested review from Bastien Montagne 2024-04-24 17:44:29 +02:00

Bastien Montagne commented

2024-04-25 10:38:47 +02:00

@blender-bot build

Bastien Montagne commented

2024-04-25 11:02:19 +02:00

Future Blender versions are likely to write more invalid pointers, which
would then trigger these asserts in older versions. I guess as long as
it is only an assert this is ok, but could be annoying in some cases.
Though with these asserts they are more likely to be caught and fixed.

Honestly, I'd rather have a CLOG_ERROR here... I suspect an assert may indeed become very annoying in the future, given how little testing (if any at all) forward compatibility gets... And the log error will also appear in release builds, not only debug ones.

> Future Blender versions are likely to write more invalid pointers, which > would then trigger these asserts in older versions. I guess as long as > it is only an assert this is ok, but could be annoying in some cases. > Though with these asserts they are more likely to be caught and fixed. Honestly, I'd rather have a `CLOG_ERROR` here... I suspect an assert may indeed become very annoying in the future, given how little testing (if any at all) forward compatibility gets... And the log error will also appear in release builds, not only debug ones.

Brecht Van Lommel commented

2024-04-26 17:43:02 +02:00

I'd rather not bother users with this message. But I guess I can make it a not abort and still show the message when asserts are enabled? Though I don't think we have an existing mechanism or convention for that.

Bastien Montagne commented

2024-05-01 11:01:01 +02:00

What's the practical difference between 'use CLOG' and 'assert with no abort', from a user PoV?

Am also not sure how noisy that would be for the user - it's only in console, and is not expected to happen often, especially not (hopefully!) with files produced by official releases?

And IIRC, CLOG_ERROR will be detected and cause failure of unittests steps on the buildbot (because its message starts with ERROR in the console)?

What's the practical difference between 'use `CLOG`' and 'assert with no abort', from a user PoV? Am also not sure how noisy that would be for the user - it's only in console, and is not expected to happen often, especially not (hopefully!) with files produced by official releases? And IIRC, `CLOG_ERROR` will be detected and cause failure of unittests steps on the buildbot (because its message starts with `ERROR` in the console)?

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

This pull request has changes conflicting with the target branch.

source/blender/blenloader/intern/readfile.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u refactor-assert-broken-pointer:brecht-refactor-assert-broken-pointer

git checkout brecht-refactor-assert-broken-pointer

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Refactor: Blend file reading asserts for broken pointers #120997

Checkout