Page MenuHome

UDIM: Support tile sets that do not start at 1001
ClosedPublic

Authored by Jesse Yurkovich (deadpin) on Jul 9 2021, 6:00 AM.
Tags
  • Restricted Project
Tokens
"Burninate" token, awarded by AlexeyAdamitsky."Love" token, awarded by silex."Love" token, awarded by irfan."Love" token, awarded by gilberto_rodrigues."Love" token, awarded by riouxr.

Details

Summary

Blender currently assumes, and hardcodes in various places, that UDIM
tiles start at 1001. This is not required and is problematic when
working with assets produced by other software where the artist laid out
tiles differently. Blender is already capable of handling sparse tile
sets (non-contiguous tiles) so the restriction around starting at 1001
is unnecessary in general.

The primary set of changes here is to remove the assumption and
restriction that 1001 is special and must always be present. These were
mostly straightforward changes until trying to update the Python API
support…

The Python API allows scripts to update a tile's tile_number field
directly, with no other processing taking place. This is very efficient
and allows the Python's view of the tiles list to remain unchanged
during the course of the operator or while iterating over the list for
example.

But this API is actually broken in 2 ways currently. 1) When a tile
changes its tile_number it will lose the ability to lookup its cached
ibuf entry. It will show as magenta in the editor space AND the cached
entry will be orphaned as no cleanup occurs. 2) BKE_image_add_tile and
gpu_texture_create_tile_mapping both assume a sorted list of tiles to
prevent duplicate tiles from being added and in order to display them
properly. This is something that's not accounted for after python
manipulation… oops.

This patch fixes the issues above:

  • Removes assumptions on 1001 always being present and is no longer mandatory
  • Makes the Python API re-assign the cached ibuf entry in order to prevent orphans and to correctly show the image
  • Sorts the tile list before each operation that requires it. Such operations are only called infrequently and shouldn't interfere with scripting in surprising ways.

Examples using a slightly modified UDIM Monster demo file

Non-1001 starting tile in master:

This patch:

Diff Detail

Repository
rB Blender
Branch
udim_sparse (branched from master)
Build Status
Buildable 15707
Build 15707: arc lint + arc unit

Event Timeline

Jesse Yurkovich (deadpin) requested review of this revision.Jul 9 2021, 6:00 AM
Jesse Yurkovich (deadpin) created this revision.
Jesse Yurkovich (deadpin) added a project: Restricted Project.

Codewise seems fine, want to do a test to stress the code on exceptional workflows.

source/blender/blenkernel/intern/image.c
691

You removed the image source check what is actually a sanity check. Consider adding an assert on image source.

Jeroen Bakker (jbakker) requested changes to this revision.Aug 9 2021, 11:48 AM
Jeroen Bakker (jbakker) added inline comments.
source/blender/blenkernel/intern/image.c
3839

Call does not match the prototype:

static void image_assign_ibuf(Image *ima, ImBuf *ibuf, int index, int entry)
source/blender/editors/space_image/image_sequence.c
141

variable is unused.

This revision now requires changes to proceed.Aug 9 2021, 11:48 AM

Address code review feedback

Jesse Yurkovich (deadpin) marked 3 inline comments as done.Aug 10 2021, 7:47 AM
This revision is now accepted and ready to land.Aug 16 2021, 10:46 AM
Sergey Sharybin (sergey) added inline comments.
intern/cycles/blender/blender_util.h
250
intern/cycles/blender/blender_util.h
250

Sadly, 'empty': is not a member of 'BL::CollectionRef<...> so I can't quite do that. I'll hold off committing in case you want to take a second look in general.

intern/cycles/blender/blender_util.h
250

We should check with @Sergey Sharybin (sergey), but both master and cycles-x branch doesn't have this method defined.
It is a good improvement for readability and consistency, but would expect that to happen in a separate patch as it is something else and the change should be applied to other areas as well.
Best to keep a patch small.

intern/cycles/blender/blender_util.h
250

Didn't realize this is a RNA collection.
Jeroen is right: it is nice to support empty() checks there, but is not for this patch.

P.S. Note that is not only about readability in this case, we don't want to iterate list collection to see if its empty or not ;)