Details

Reviewers

Bastien Montagne (mont29)
Brecht Van Lommel (brecht)

Commits

rCcd4e6db7c707: UDIM: Support virtual filenames
rB180b66ae8a1f: UDIM: Support virtual filenames

Summary

This implements the design detailed in T92696 to support virtual
filenames for UDIM textures. Currently, the following 2 substitution
tokens are supported:

Token	Meaning	Support
<UDIM>	1001 + u-tile + v-tile * 10	MaterialX / USD / OIIO
<UVTILE>	Equivalent to u<u-tile + 1>_v<v-tile + 1>	MaterialX

Example for u-tile of 3 and v-tile of 1:
filename.<UDIM>_ver0023.png --> filename.1014_ver0023.png
filename.<UVTILE>_ver0023.png --> filename.u4_v2_ver0023.png

For image loading, the existing workflow is unaffected. A user can
select one or more image files, belonging to one or more UDIM tile sets,
and have Blender load them all as it does today. Now the <UVTILE> format
is "guessed" just as the <UDIM> format was guessed before.

If guessing fails, the user can simply go into the Image Editor and type
the proper substitution in the filename. Once typing is complete,
Blender will reload the files and correctly fill the tiles. This
workflow is new as attempting to fix the guessing in current versions
did not really work and the user was often stuck with a confusing
situation.

For image saving, the existing workflow is changed slightly. When saving
currently, a user has to be sure to type the filename of the first tile
(e.g. filename.1001.png) to save the entire UDIM set. The number could
differ if they start at a different tile etc. This is confusing. Now,
the user should type a filename containing the appropriate substitution
token. By default Blender will fill in a default name using the <UDIM>
token but the user is free to save out images using <UVTILE> if they
wish.

Notes
The design task mentions better support for importers/exporters like
USD which require handling of the virtual filepaths natively. Better
support is still planned but not included in this patch and will be
added later. Probably after D13297 lands.

The choice of the 2 substitution tokens supported was based solely on
what USD/MaterialX requires and which are rather prevalent in the
ecosystem already. More tokens can be added in the future if a strong
need arises for them.

Diff Detail

Repository

rB Blender

Branch

udim_virtual (branched from master)

Build Status

Buildable 19346
Build 19346: arc lint + arc unit

Event Timeline

Jesse Yurkovich (deadpin) requested review of this revision.Oct 31 2021, 11:23 PM

Jesse Yurkovich (deadpin) created this revision.

Harbormaster completed remote builds in B18354: Diff 44181.Oct 31 2021, 11:23 PM

Jesse Yurkovich (deadpin) mentioned this in T92696: UDIM: Support virtual filenames for tile sets.Oct 31 2021, 11:24 PM

Jesse Yurkovich (deadpin) edited the summary of this revision. (Show Details)

Handle the proposed "fixup" workflow by implementing proper "Reload" support
Use correct extern declarations

Harbormaster completed remote builds in B18837: Diff 45042.Nov 21 2021, 12:23 AM

Updates

Only allow '<' and '>' characters from the File-Selector code path
Handle relative paths when doing the "fixup" workflow
Add versioning to allow tests and older files to work (all tests pass)
TODO: Needed to revert my prior change to cycles/scene/image.cpp since that file is not permitted to use Blender APIs (I think)

Harbormaster completed remote builds in B19203: Diff 45643.Dec 5 2021, 8:03 AM

Have not looked in detail, seems pretty reasonable overall.

Would be nice if we could get the Find Missing Files operator working with this somehow.

intern/cycles/scene/image.cpp
385–386	We can't really practically speaking. Cycles needs to work separate from Blender and making a shared library seems overkill.
source/blender/blenkernel/intern/image.c
4088–4090	Don't hardcode 8, use `strlen` instead.
source/blender/blenloader/intern/versioning_300.c
801	Just `if(ima->source == IMA_SRC_TILED) { BKE_image_ensure_udim_token... }` is simpler.

Support "Report missing files" and "Find missing files" operators
Add a bit of documentation for the newly added BKE_image functions
Feedback

Harbormaster completed remote builds in B19346: Diff 45903.Dec 10 2021, 9:57 AM

Thanks for the reminder about the find missing files operator! That workflow should now work after the latest update.

I think the biggest todo remaining is what to do about the BKE_image_load existence check -- I can ensure at least 1 concrete file exists for sure, but it's a heavy operation to do. Should we just do it (would use BKE_image_get_udim_strformat + BKE_image_udim_exists)?

In D13057#357674, @Jesse Yurkovich (deadpin) wrote:

I think the biggest todo remaining is what to do about the BKE_image_load existence check -- I can ensure at least 1 concrete file exists for sure, but it's a heavy operation to do. Should we just do it (would use BKE_image_get_udim_strformat + BKE_image_udim_exists)?

From what I can tell this is used in a few places:

Image > Open
Drag & drop of file into Blender
USD / Collada Import
Python API

For the first two cases performance is a non-issue. For native or Python importers, it's probably not a big issue either. Maybe in a future where we dynamically load USD assets it could become a problem.

I guess in most cases there will exists a low tile number and it will be quick to find. And if it's missing it's an error anyway. So seems reasonable to just check for existence of a UDIM tile.

source/blender/blenkernel/intern/bpath.c
227–236	I don't think we should treat every path as potentially containing UDIM tokens. `BPathForeachPathData` could have some flag set for tiled image file paths. Another option to makes these operator oblivious to UDIMs would be: Add a `BKE_BPATH_FOREACH_PATH_RESOLVE_TOKEN` flag used by these operators Make the image code return the path of the first tile if that flag is set If that path is modified, put the token back into the path

Feedback, fixes, and renames

Create a new BPATH_FOREACH flag to better isolate the changes and per feedback
Put back the image open/existence check inside BKE_image_load
Remove 'udim' from the API and use the existing 'tile' convention

Harbormaster completed remote builds in B19388: Diff 45975.Dec 13 2021, 9:20 AM

I guess at this point this is no longer a WIP prototype, but ready for review?

@Bastien Montagne (mont29), does the generic file path handling look acceptable to you? I think it's pretty well abstraced like this with minimal impact on the rest of the code.

intern/cycles/blender/util.h
36	Rename substitute to resolve for consistency.
intern/cycles/scene/image.cpp
385–386	Use `/**/` comment style to follow code conventions. The meaning of the comment is now also unclear to me. Does this now support all the same token types as Blender itself? If so let's mention that and leave out "for now".
source/blender/blenkernel/BKE_image.h
445	f you want to put a warning that this can be slow and should be used with care I think that makes sense. However unspecified rare circumstances are not clear.
source/blender/blenkernel/intern/image.c
288	Put the resolved filepath in a variable on the stack instead of temporarily modifying `ima->filepath`. I think it's good practice to keep this function reentrant, even if there is no situation right now where multiple threads call this concurrently. Only if `BKE_bpath_foreach_path_fixed_process` returns true should we modify the actual filepath. It may be most readable to structure the code like this: if (ima->source == IMA_SRC_TILED && (flag & BKE_BPATH_FOREACH_PATH_RESOLVE_TOKEN) != 0) { ... result = BKE_bpath_foreach_path_fixed_process(bpath_data, filepath); ... } else { result = BKE_bpath_foreach_path_fixed_process(bpath_data, ima->filepath); }

This revision now requires changes to proceed.Dec 13 2021, 2:42 PM

Feedback

Harbormaster completed remote builds in B19404: Diff 46001.Dec 14 2021, 1:03 AM

Jesse Yurkovich (deadpin) retitled this revision from WIP: UDIM: Prototype for udim virtual filenames to UDIM: Prototype for udim virtual filenames.Dec 14 2021, 1:05 AM

Jesse Yurkovich (deadpin) edited the summary of this revision. (Show Details)

Jesse Yurkovich (deadpin) marked 5 inline comments as done.Dec 14 2021, 1:11 AM

Brecht Van Lommel (brecht) added inline comments.Dec 14 2021, 1:14 AM

intern/cycles/scene/image.cpp
385	I'm being very pedantic now, but the convention in Cycles is: /* Since we don't have information about the exact tile format used in this code location, * just attempt all replacement patterns that Blender supports. / Starting with `/*` is a doxygen thing for documenting a function, not comments inside functions.
source/blender/blenkernel/intern/image.c
295–301	Again being pedantic, but I think it's better to move this part and the relevant local variables inside `if (ima->source == IMA_SRC_TILED && (flag & BKE_BPATH_FOREACH_PATH_RESOLVE_TOKEN) != 0) {`.

Feedback

Harbormaster completed remote builds in B19406: Diff 46004.Dec 14 2021, 1:32 AM

Brecht Van Lommel (brecht) accepted this revision.Dec 14 2021, 1:59 AM

Brecht Van Lommel (brecht) added 1 blocking reviewer(s): Bastien Montagne (mont29).

Jesse Yurkovich (deadpin) retitled this revision from UDIM: Prototype for udim virtual filenames to UDIM: Support virtual filenames.Dec 21 2021, 9:15 AM

Update with master

Harbormaster completed remote builds in B19646: Diff 46411.Dec 27 2021, 1:24 PM

Globally fine with this patch, my main concern being the change to file_filename_enter_handle, which allows tokens for all cases in file browser path validation function (see note below).

Note that I only checked in detail those changes related to BLI_filename_make_safe/_ex, not the rest of the patch.

PS: I don't think we need to rush this in before BCon2, such relatively limited changes remain acceptable in early BCon2 ("Improve and stabilize") as well.

source/blender/blenlib/BLI_path_util.h
250–274	Missing doc about new `allow_tokens` parameter.
source/blender/editors/space_file/file_ops.c
2626	This is my main remaining issue with this patch. IMHO accepting 'tokens' in file path here should be the exception, and not the rule. Ideally, this should be specified as a new option of the filebrowser, something like `FILE_PATH_TOKENS_ALLOW`, added to the `eFileSel_Params_Flag` enum e.g. Then one can control when the file browser should allow those tokens. At the very least, I would expect a comment here explaining why we need this, with TODO note about how to properly restrict it to cases that need it in the future?

This revision now requires changes to proceed.Dec 27 2021, 4:14 PM

Added a new option to limit the tokens to just the 2 operators which require it.

Only allow tokens from the image open/save-as operators

Harbormaster completed remote builds in B19669: Diff 46450.Dec 28 2021, 11:51 PM

Thanks, LGTM now.

This revision is now accepted and ready to land.Dec 29 2021, 9:34 AM

Looks ready to commit and add to the release notes.

Closed by commit rB180b66ae8a1f: UDIM: Support virtual filenames (authored by Jesse Yurkovich (deadpin)). · Explain WhyJan 3 2022, 6:29 AM

This revision was automatically updated to reflect the committed changes.

Jesse Yurkovich (deadpin) added a commit: rB180b66ae8a1f: UDIM: Support virtual filenames.

Steffen Dünner (SteffenD) added a subscriber: Steffen Dünner (SteffenD).Jan 3 2022, 3:04 PM

Brecht Van Lommel (brecht) added a commit: rCcd4e6db7c707: UDIM: Support virtual filenames.Jan 27 2022, 11:38 PM

Martin Malmqvist (Malmqvist) awarded a token.Mar 1 2022, 8:57 AM

UDIM: Support virtual filenames
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 45903

intern/cycles/blender/shader.cpp

intern/cycles/blender/util.h

intern/cycles/scene/image.cpp

source/blender/blenkernel/BKE_image.h

source/blender/blenkernel/intern/bpath.c

source/blender/blenkernel/intern/image.c

source/blender/blenkernel/intern/image_save.c

source/blender/blenlib/BLI_path_util.h

source/blender/blenlib/intern/path_util.c

source/blender/blenloader/intern/versioning_300.c

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_image/image_ops.c

source/blender/editors/space_image/image_sequence.c

UDIM: Support virtual filenamesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 45903

intern/cycles/blender/shader.cpp

intern/cycles/blender/util.h

intern/cycles/scene/image.cpp

source/blender/blenkernel/BKE_image.h

source/blender/blenkernel/intern/bpath.c

source/blender/blenkernel/intern/image.c

source/blender/blenkernel/intern/image_save.c

source/blender/blenlib/BLI_path_util.h

source/blender/blenlib/intern/path_util.c

source/blender/blenloader/intern/versioning_300.c

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_image/image_ops.c

source/blender/editors/space_image/image_sequence.c

UDIM: Support virtual filenames
ClosedPublic
Actions

Revision Contents
Changeset List