Regression: Multires data is not loaded if it comes from an external file #107151

New Issue

Closed

opened 2023-04-20 01:56:17 +02:00 by Juan Pablo Bouza · 8 comments

Juan Pablo Bouza commented

2023-04-20 01:56:17 +02:00

Member

System Information
Operating system: Kubuntu 20.04
Graphics card: Geforce 2080 rtx

Blender Version
version: 3.5.0, branch: blender-v3.5-release, commit date: 2023-03-29 02:56, hash: 1be25cfff1, type: release

Caused by 3a3d9488a1

If you sculpt an object with Multires and save the data as an external BTX file, when you reload the file, the Multires Data is no longer loaded.

Steps:

Add multures to the default cube
Subdivide and Sculpt something out
Save the external BTX from the multires modifier
Save and reload blend file

**System Information** Operating system: Kubuntu 20.04 Graphics card: Geforce 2080 rtx **Blender Version** version: 3.5.0, branch: blender-v3.5-release, commit date: 2023-03-29 02:56, hash: 1be25cfff18b, type: release Caused by 3a3d9488a1633bfefabbab422dd22283bca02626 If you sculpt an object with Multires and save the data as an external BTX file, when you reload the file, the Multires Data is no longer loaded. Steps: 1) Add multures to the default cube 2) Subdivide and Sculpt something out 3) Save the external BTX from the multires modifier 4) Save and reload blend file

Juan Pablo Bouza added the

labels 2023-04-20 01:56:18 +02:00

Philipp Oeser commented

2023-04-20 11:29:43 +02:00

Member

Can confirm, will check

Can confirm, will check

Philipp Oeser added

and removed

labels 2023-04-20 11:29:51 +02:00

Philipp Oeser commented

2023-04-20 12:52:10 +02:00

Member

Caused by 3a3d9488a1

Caused by 3a3d9488a1633bfefabbab422dd22283bca02626

Philipp Oeser changed title from ~~Multires data is not loaded if it comes form an external file~~ to Regression: Multires data is not loaded if it comes form an external file

2023-04-20 12:52:30 +02:00

Philipp Oeser added

Sculpt, Paint & Texture

and removed

labels 2023-04-20 12:52:54 +02:00

Philipp Oeser commented

2023-04-20 14:07:51 +02:00

Member

Seems layerRead_mdisps runs properly, but maybe it is not playing nice with the mutable thingie from the culprit commit.

I was (blindly) testing the following (to no avail):


diff --git a/source/blender/blenkernel/intern/customdata.cc b/source/blender/blenkernel/intern/customdata.cc
index e0a60a6925f..2eb722fbde8 100644
--- a/source/blender/blenkernel/intern/customdata.cc
+++ b/source/blender/blenkernel/intern/customdata.cc
@@ -4618,7 +4618,7 @@ void CustomData_external_read(CustomData *data, ID *id, eCustomDataMask mask, co
 
       if (blay) {
         if (cdf_read_layer(cdf, blay)) {
-          if (typeInfo->read(cdf, layer->data, totelem)) {
+          if (typeInfo->read(cdf, CustomData_get_layer_for_write(data, eCustomDataType(layer->type), totelem), totelem)) {
             /* pass */
           }
           else {

Seems `layerRead_mdisps` runs properly, but maybe it is not playing nice with the mutable thingie from the culprit commit. I was (blindly) testing the following (to no avail): ``` diff --git a/source/blender/blenkernel/intern/customdata.cc b/source/blender/blenkernel/intern/customdata.cc index e0a60a6925f..2eb722fbde8 100644 --- a/source/blender/blenkernel/intern/customdata.cc +++ b/source/blender/blenkernel/intern/customdata.cc @@ -4618,7 +4618,7 @@ void CustomData_external_read(CustomData *data, ID *id, eCustomDataMask mask, co if (blay) { if (cdf_read_layer(cdf, blay)) { - if (typeInfo->read(cdf, layer->data, totelem)) { + if (typeInfo->read(cdf, CustomData_get_layer_for_write(data, eCustomDataType(layer->type), totelem), totelem)) { /* pass */ } else { ```

Philipp Oeser commented

2023-04-20 14:08:03 +02:00

Member

CC @HooglyBoogly

CC @HooglyBoogly

Hans Goudey self-assigned this 2023-04-20 14:31:32 +02:00

Hans Goudey added

and removed

labels 2023-04-20 14:31:39 +02:00

Hans Goudey commented

2023-04-20 23:44:39 +02:00

Member

It looks like the code before the const correctness cleanup relied on some non-threadsafe logic for reading the original multires displacement. We can get that back with the following diff:

diff --git a/source/blender/blenkernel/intern/multires.cc b/source/blender/blenkernel/intern/multires.cc
index 0cd8cf5079c..99a347e0f20 100644
--- a/source/blender/blenkernel/intern/multires.cc
+++ b/source/blender/blenkernel/intern/multires.cc
@@ -1520,8 +1520,9 @@ void multires_ensure_external_read(struct Mesh *mesh, int top_level)
     return;
   }
 
-  MDisps *mdisps = static_cast<MDisps *>(
-      CustomData_get_layer_for_write(&mesh->ldata, CD_MDISPS, mesh->totloop));
+  /* Modify the data array from the original mesh, not the evaluated mesh. */
+  MDisps *mdisps = const_cast<MDisps *>(
+      static_cast<const MDisps *>(CustomData_get_layer(&mesh->ldata, CD_MDISPS)));
   if (mdisps == nullptr) {
     mdisps = static_cast<MDisps *>(
         CustomData_add_layer(&mesh->ldata, CD_MDISPS, CD_SET_DEFAULT, mesh->totloop));

However, that's probably not the right thing to do, since this reading isn't threadsafe, and gives a bunch of memory leaks if a bunch of objects with multires modifiers share the same mesh:

I wonder if there's a better place to call multiresModifier_ensure_external_read rather than in the evaluation of the multires modifier. Ideally it would be called on the original mesh directly. @Sergey, any idea about that?

It looks like the code before the const correctness cleanup relied on some non-threadsafe logic for reading the original multires displacement. We can get that back with the following diff: ```diff diff --git a/source/blender/blenkernel/intern/multires.cc b/source/blender/blenkernel/intern/multires.cc index 0cd8cf5079c..99a347e0f20 100644 --- a/source/blender/blenkernel/intern/multires.cc +++ b/source/blender/blenkernel/intern/multires.cc @@ -1520,8 +1520,9 @@ void multires_ensure_external_read(struct Mesh *mesh, int top_level) return; } - MDisps *mdisps = static_cast<MDisps *>( - CustomData_get_layer_for_write(&mesh->ldata, CD_MDISPS, mesh->totloop)); + /* Modify the data array from the original mesh, not the evaluated mesh. */ + MDisps *mdisps = const_cast<MDisps *>( + static_cast<const MDisps *>(CustomData_get_layer(&mesh->ldata, CD_MDISPS))); if (mdisps == nullptr) { mdisps = static_cast<MDisps *>( CustomData_add_layer(&mesh->ldata, CD_MDISPS, CD_SET_DEFAULT, mesh->totloop)); ``` However, that's probably not the right thing to do, since this reading isn't threadsafe, and gives a bunch of memory leaks if a bunch of objects with multires modifiers share the same mesh: ![image](/attachments/5c8f0983-151a-4f60-8766-fbe89837111a) I wonder if there's a better place to call `multiresModifier_ensure_external_read` rather than in the evaluation of the multires modifier. Ideally it would be called on the original mesh directly. @Sergey, any idea about that?

570 KiB

Sergey Sharybin commented

2023-04-21 10:34:35 +02:00

Owner

@HooglyBoogly I am not really sure either what is the best place to read it for the original mesh. Maybe somewhere after the file load, and on the external path property update? And all other places which are not immediately obvious.

However, that's probably not the right thing to do, since this reading isn't threadsafe, and gives a bunch of memory leaks if a bunch of objects with multires modifiers share the same mesh:

Not sure how often this happens setup happens in practice, and would imagine the old code had the same issues.
Can probably add some lock or something.

Thing is, not sure how much time we want to dedicate on making the current external data to work reliably. It was done to support "variants" of the sculpted data. More proper result is to support multires layers as a built-in core feature.

@HooglyBoogly I am not really sure either what is the best place to read it for the original mesh. Maybe somewhere after the file load, and on the external path property update? And all other places which are not immediately obvious. > However, that's probably not the right thing to do, since this reading isn't threadsafe, and gives a bunch of memory leaks if a bunch of objects with multires modifiers share the same mesh: Not sure how often this happens setup happens in practice, and would imagine the old code had the same issues. Can probably add some lock or something. Thing is, not sure how much time we want to dedicate on making the current external data to work reliably. It was done to support "variants" of the sculpted data. More proper result is to support multires layers as a built-in core feature.

Juan Pablo Bouza commented

2023-04-21 14:11:28 +02:00

Author

Member

Guys, this feature has been in Blender since it first landed, I don't know about supporting variants. One key aspect of the BTX files is that if you have a 20gb sculpt, you just save it externally so that the different versions of the blend file don't carry with that heavy weight.
So even you supported Sculpt Layers, which would be great, most of the time saving those layers inside of the blend file would be a very bad option, specially if it's a very dense sculpt.

Guys, this feature has been in Blender since it first landed, I don't know about supporting variants. One key aspect of the BTX files is that if you have a 20gb sculpt, you just save it externally so that the different versions of the blend file don't carry with that heavy weight. So even you supported Sculpt Layers, which would be great, most of the time saving those layers inside of the blend file would be a very bad option, specially if it's a very dense sculpt.

Hans Goudey changed title from ~~Regression: Multires data is not loaded if it comes form an external file~~ to Regression: Multires data is not loaded if it comes from an external file

2023-04-21 14:58:26 +02:00

Sergey Sharybin commented

2023-04-21 15:05:41 +02:00

Owner

@jpbouza-4 Hey. This is definitely a good point.

The layering does not cancel out the external data. There is also a concern of always reading the data from the file, regardless of whether the modifier will need it or not). I did not go into those topics in the previous reply because it seemed more practical to restore old behavior without for the short-term solution, before going deeper into the code.

The main point was that if the proper fix takes too much time and internal redesign to fit the current state of the feature, it might be better not to do those changes as some of the (not-currently-finalised-planned) sculpt development would need to heavily change the current design yet again.

Hope that makes sense.

@jpbouza-4 Hey. This is definitely a good point. The layering does not cancel out the external data. There is also a concern of always reading the data from the file, regardless of whether the modifier will need it or not). I did not go into those topics in the previous reply because it seemed more practical to restore old behavior without for the short-term solution, before going deeper into the code. The main point was that if the proper fix takes too much time and internal redesign to fit the current state of the feature, it might be better not to do those changes as some of the (not-currently-finalised-planned) sculpt development would need to heavily change the current design yet again. Hope that makes sense.

Philipp Oeser referenced this issue

2023-04-26 10:00:32 +02:00

External Multires data not loading #107341

Hans Goudey referenced this issue from a commit

2023-05-12 19:49:11 +02:00

Fix #107151: Multires data from external file is not loaded

Hans Goudey closed this issue

2023-05-12 19:49:12 +02:00

Blender Bot added

and removed

labels 2023-05-12 19:49:13 +02:00

Sign in to join this conversation.

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

4 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#107151

No description provided.