Fix T99388: Obey relative path option when saving UDIMs
ClosedPublic
Actions

Authored by Jesse Yurkovich (deadpin) on Jul 6 2022, 8:52 AM.

Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)

Maniphest Tasks

T99388: Saving external image to udim makes path absolute

Commits

rBf25620187620: Fix T99388: Obey relative path option when saving UDIMs

Summary

Ensure that the Image maintains the proper file path after saving all
the individual tiles.

The image_save_post function is unaware that the filepath it receives
is only for a single tile, not the entire Image, and happily keeps
setting ima->filepath to the concrete filepath for each tile.

There were 2 problems with the code that attempted to correct the
Image filepath back to the UDIM virtual form:

It would trample the "relative" directory that might have been set
It would do the wrong thing if no tiles could be saved at all

Diff Detail

Repository: rB Blender

Event Timeline

Jesse Yurkovich (deadpin) requested review of this revision.Jul 6 2022, 8:52 AM

Jesse Yurkovich (deadpin) created this revision.

Harbormaster completed remote builds in B22870: Diff 53285.Jul 6 2022, 8:52 AM

Jesse Yurkovich (deadpin) added a task: T99388: Saving external image to udim makes path absolute.Jul 6 2022, 8:52 AM

It would do the wrong thing if no tiles could be saved at all

What exactly is wrong, what is the proper/expected thing to do if any of tiles has failed to save?

As for the implementation I can not say such store-modify-restore approach is the cleanest.
From my understanding, the opts->filepath needs to be restored due to BKE_image_set_filepath_from_tile_number(). So, why not to implement this as:

ImageSaveOptions tile_opts = *opts;
BKE_image_set_filepath_from_tile_number(
    tile_opts.filepath, udim_pattern, tile_format, tile->tile_number);
iuser->tile = tile->tile_number;
ok = image_save_single(reports, ima, iuser, &tile_opts, &colorspace_changed);

This will also allow to do things like tile_opts.relative = false; and take care of making image's path relative after the tiles has been written.

What we can also do is to skip modification of ima->filepath in the image_save_post with a comment that it will be handled somewhere else. Seems to be less intrusive than splitting the opts->do_newpath into "do new path for image buffer" (which we do want) and "do new path for image itself" which we don't want here.

P.S. On a semi-related topic, I think the ImageSaveOptions *opts and ImageFormatData *imf should be passed by a const pointer to the mage saving functions.

source/blender/blenkernel/intern/image_save.cc
656	`dir` is rather generic name. What is even worse: it changes meaning throughout its lifetime.
660	If you need to keep track of number of saved tiles, then name it `saved_tiles_num`. Otherwise `have_saved_tiles` is more clear name. However, I am not sure why the `ok` can be kept as is without introducing an extra variable which for the same purpose. It can be `if (ok) { do path remapping }`

This revision now requires changes to proceed.Jul 6 2022, 3:26 PM

Feedback
Refactor

Harbormaster completed remote builds in B22878: Diff 53336.Jul 7 2022, 8:57 AM

In D15384#416083, @Sergey Sharybin (sergey) wrote:

It would do the wrong thing if no tiles could be saved at all

What exactly is wrong, what is the proper/expected thing to do if any of tiles has failed to save?

For normal images, if saving fails (ok = false), the ima->filepath is left unchanged/empty. Before this patch, a tiled image would have its ima->filepath unconditionally updated even if all tiles failed to save somehow. The first idea of this patch was to return to the pre 3.1 behavior of updating the ima->filepath if any tile was successfully written(as a result of the code inside image_save_post)... But now I think that's probably not a good idea to do either. Setting the filepath when some tiles aren't written is confusing and could lead to problems saving unmodified changes actually.

I think the following is the proper/expected thing to do if any tiles fail to save. Consider the following cases when trying to save to PathB:

	all tiles ok	any tile not ok
ima->filepath is currently empty	set to new PathB	keep empty
ima->filepath is currently PathA	set to new PathB	keep PathA

I've uploaded a revision that implements that table and separates out the problematic part. It's much simpler now but did add a conditional inside the main image_save_post call.

Good feedback about copying ImageSaveOptions. Will implement the "const" changes for a separate cleanup commit.

Thanks for the explanation! What you describe totally makes sense now. Suggestion would be to copy-paste this information to the commit message, so that it is not needed to dig comment in the review section to get this knowledge.

Have a little suggestion about the function naming.

There is some importance to this fix here at the studio. The remaining feedback is not more or less cosmetic and can be addressed without an extra review iteration, saving some time :)

source/blender/blenkernel/intern/image_save.cc
256	Having it a polymorphic function is a bit confusing here: the semantic is so much different between different implementations of this function. Better name would be `image_update_file_path_after_save()` or something like this. Maybe you can think of a shorter/clearer name as well.