Page MenuHome

Fix T97251: Store generated type information for each UDIM tile
ClosedPublic

Authored by Jesse Yurkovich (deadpin) on May 8 2022, 11:03 AM.

Details

Summary

Various situations can lead to un-saved UDIM tiles potentially losing
their contents. The most notable situation is a save and re-load of a
.blend file that has "generated" UDIM tiles that haven't been written to
disk yet. Normal "generated" images are reconstructed on demand in these
circumstances but UDIM tiles do not retain the information required for
reconstruction and empty tiles are presented to the user.

This patch stores the generated type information for each tile to solve
this particular issue.

However, in order to distinguish between a tile that should be
reconstructed vs. a tile that should remain empty because loading
failed, a new flag is used. It's usage is as follows:

  • Every tile's gen_flag field is set to 0 on creation and when loaded from disk (historically only 1bit of this field was used for the indication of float buffers)
  • Each time a tile is filled with generated content we set a new IMA_GEN_TILE flag
  • Each time a tile is saved to disk we remove the IMA_GEN_TILE flag
  • When requesting an ibuf: If the ibuf is null, we check to see if IMA_GEN_TILE is set. If it is set, go ahead and re-create the tile. Otherwise, do nothing.

I tried an alternative code change where I grouped all 6 of the image generation fields into a common structure to reduce duplication. This is fine up until versioning where I would be unable to transfer from the old DNA struct to the new one without leaving the prior 6 fields in place... (and marking them as DNA_DEPRECATED).

Diff Detail

Repository
rB Blender
Branch
udim-generated (branched from master)
Build Status
Buildable 22017
Build 22017: arc lint + arc unit

Event Timeline

Jesse Yurkovich (deadpin) requested review of this revision.May 8 2022, 11:03 AM
Jesse Yurkovich (deadpin) created this revision.
  • Add rna update methods
Jesse Yurkovich (deadpin) planned changes to this revision.Jul 7 2022, 7:25 AM

This should wait until another change in the area is taken care of first. I'll need to adapt this patch afterwards.

Brecht Van Lommel (brecht) requested changes to this revision.Jul 29 2022, 2:21 PM

My understanding is that all types of images have at least one ImageTile. So I think it may be better to deprecate the members in Image and fully move them to ImageTile? We could do this while still preserving API compatibility.

With the current implementation, I think changing the image type from non-tiled to tiled will also lose the information. We could add some synchronization mechanism, but it's easier if they are stored in a single place to begin with.

This revision now requires changes to proceed.Jul 29 2022, 2:21 PM
Jesse Yurkovich (deadpin) edited the summary of this revision. (Show Details)
  • Feedback and fixes

My understanding is that all types of images have at least one ImageTile. So I think it may be better to deprecate the members in Image and fully move them to ImageTile? We could do this while still preserving API compatibility.

Yes, everything should have a tile. Changed so that the tiles are now the sole source for the generated information while keeping back compat.

With the current implementation, I think changing the image type from non-tiled to tiled will also lose the information. We could add some synchronization mechanism, but it's easier if they are stored in a single place to begin with.

Hmm, yes, Source changes are problematic (even before this patch really). The combinations are pretty nuanced and caused minor changes in many places, but I think I've addressed the most egregious issues. See the table below for what happens in each situation now.

Summary of changes since last revision:

rna_image.c / DNA_image_types.h / versioning_300.c

  • Deprecate old gen* fields
  • Forward incoming RNA calls for the Image gen* fields to the Image 1st tile instead
  • Properly refresh the exact generated tile if the generated information is modified from Python
  • Versioning for old files

image_save.cc

  • Ensure we remove the IMA_GEN_TILE bit when saving out non-UDIM files now too

image.cc

  • Added function to allocate tiles with the necessary defaults (mimicking what Image used to have)
  • add_ibuf_size -> add_ibuf_for_tile with different parameters to reduce duplication at each call site
  • A set of BKE_image_signal changes in order to implement the below table for the case IMA_SIGNAL_SRC_CHANGE

The Signal changes aren't too bad but it's sometimes difficult to reason about because at the time that this function is called the source has already been changed to the new one.

Scenariomasterpatch
Generated to SingleEmpty image (Single requires a physical file to be present)Empty image (Single requires a physical file to be present)
Generated to UDIMEmpty imageUses the 1st tile's gen info
Single to GeneratedUses the Image gen info (otherwise uses default 1k UV grid)Uses the 1st tile's gen info (otherwise uses default 1k UV grid)
UDIM to GeneratedUses the Image gen info (otherwise uses default 1k UV grid)Uses the 1st tile's gen info (otherwise uses default 1k UV grid)
Single to UDIMDisplays loaded image in a tile - but filepath is not virtualDisplays loaded image in a tile - Path is virtualized if possible and may load the remainder of the UDIM set
UDIM to SingleIf 1st tile is from a physical file -> Displays empty imageIf 1st tile is from a physical file -> Uses 1st tile's concrete file
UDIM to SingleIf 1st tile is still generated -> Displays empty image (Generated to Single scenario)If 1st tile is still generated -> Displays empty image (Generated to Single scenario)

Thanks for looking at this so thoroughly.

This revision is now accepted and ready to land.Aug 3 2022, 7:56 PM

Is this fix appropriate for 3.3 or just master?

I would put in in master since it's somewhat risky and not that critical a bug.

@Campbell Barton (campbellbarton) Should be fixed now. Sorry for that - should have used my linux VM to double-check my build.