Page MenuHome

Add support for tiled images and the UDIM naming scheme
ClosedPublic

Authored by Lukas Stockner (lukasstockner97) on Jun 28 2018, 12:43 AM.
Tokens
"Like" token, awarded by eobet."Love" token, awarded by Artsolit."Love" token, awarded by Bit."Like" token, awarded by knightknight."Love" token, awarded by fin.eskimo."Party Time" token, awarded by Zino."Mountain of Wealth" token, awarded by Pipeliner."Party Time" token, awarded by ostapblender."Love" token, awarded by Dogway."Love" token, awarded by johantri."Love" token, awarded by roman13."Love" token, awarded by Maged_afra."Love" token, awarded by reecerobinson."Love" token, awarded by bintang."Pterodactyl" token, awarded by Grische."Yellow Medal" token, awarded by mywa880."Mountain of Wealth" token, awarded by Brandon777."Party Time" token, awarded by sam_vh."Love" token, awarded by mistajuliax."Love" token, awarded by andruxa696."Mountain of Wealth" token, awarded by philstopford."Love" token, awarded by GageChristopherRenn."Love" token, awarded by Frozen_Death_Knight."Love" token, awarded by Shimoon."Love" token, awarded by rboxman."Love" token, awarded by bnzs."Love" token, awarded by juantxo."Love" token, awarded by Treki26."Love" token, awarded by Vidu."Love" token, awarded by juang3d.

Details

Summary

This patch contains the work that I did during my week at the Code Quest - adding support for tiled images to Blender.

With this patch, images now contain a list of tiles. By default, this just contains one tile, but if the source type is set to Tiled, the user can add additional tiles. When acquiring an ImBuf, the tile to be loaded is specified in the ImageUser.
Therefore, code that is not yet aware of tiles will just access the default tile as usual.

The filenames of the additional tiles are derived from the original filename according to the UDIM naming scheme - the filename contains an index that is calculated as (1001 + 10*<y coordinate of the tile> + <x coordinate of the tile>), where the x coordinate never goes above 9.
Internally, the various tiles are stored in a cache just like sequences. When acquired for the first time, the code will try to load the corresponding file from disk. Alternatively, a new operator can be used to initialize the tile similar to the New Image operator.

The following features are supported so far:

  • Automatic detection of tiled images when selecting a 1001 tile in the Image Open operator
  • Saving all tiles
  • Reloading all tiles
  • Adding and removing tiles
  • Filling tiles with generated images
  • Drawing all tiles in the Image Editor
  • Viewing a tiled grid even if no image is selected
  • Rendering tiled images in Cycles (in SVM mode)
  • Automatically skipping loading of unused tiles in Cycles
  • Rendering tiled images in Eevee
  • 2D texture painting (also across tiles)
  • 3D texture painting (also across tiles, only limitation: individual faces can not cross tile borders)
  • Assigning custom labels to individual tiles (drawn in the Image Editor instead of the ID)
  • Per-tile resolutions and aspect ratios

The current state of this patch is that tiled images are usable, but there probably still are quite a few bugs.

The most important ToDo is support for tiled textures in the regular viewport. Also, OSL mode in Cycles doesn't work yet.

Of course, there also are many remaining areas that still need to be changed to support tiled textures (for example: UV editing tools, baking, dynamic paint...), but this could be added later.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Juan Gea (juang3d), I'd be fine with removing features at first if they're blocking the patch (see e.g. Workbench engine support), but now the remaining issues (Tile UI and Packing) are very basic stuff that I can't remove.

Awesome, thanks @Lukas Stockner (lukasstockner97) !

It's awesome that you manage to fix the painting related bug :) Now if I understand correctly what remains to be solved are minor things, so that's great :)
I'll apply this to our build as soon as you say it's ready to be tested/used :)

Thanks!

This update completely reworks the tile UI.

The property panel of the Image and UV editors now shows a list of tiles in the Image tab.
From here, you can add, remove and rename tiles and fill any tiles that don't have an image in them yet.

Also, this update fixes a memory corruption bug in the tile removal code and, as usual, rebases to the current master.

Thanks @Lukas Stockner (lukasstockner97) just applied it to our build so people can start playing with it :)

Juan Gea (juang3d) added a comment.EditedNov 26 2019, 3:30 PM

Ok, after testing it in our build I think there is some memory leak related to this, we tried a texture that comes with the scatter addon and it crashes with this:

CUDA error: Illegal address in cuCtxSynchronize(), line 1898

It's not a matter of the addon because I isolated the trouble and related it with textures, so something must be leaking memory.

i'm going to remove the patch for the time being, I'll try to get a scene with the trouble.

Sometimes the render gets cancelled, other times the viewport render works, but F12 shows a pink color in the object with the shader, which cannot be because the textures are packed, if I unpack them the problem is the same, so something is weird.

I detected the problem with the Scatter addon from @bd3d, btu agian, the problem exists without the adodn, and it only affects GPU render, it does not affect CPU render.

If I get more information I'll publish it here.

EDIT: I just want to add that I can confirm that the problem is related to this patch, I just removed it and everything went fine.

Hi @Juan Gea (juang3d)

Not sure if it might be related, but I reverted the virtual destructors for anything inheriting from ImageSlotTextureNode so that only ImageTextureNode has the new UDIM destructor update. I found that textures like environment maps weren't freeing properly which might be what you're running into, although it was asserting for me but I am building in debug mode and haven't tried a release build yet.

Hi @Juan Gea (juang3d)

Not sure if it might be related, but I reverted the virtual destructors for anything inheriting from ImageSlotTextureNode so that only ImageTextureNode has the new UDIM destructor update. I found that textures like environment maps weren't freeing properly which might be what you're running into, although it was asserting for me but I am building in debug mode and haven't tried a release build yet.

Ok, I’ll try again later, at least I know how to reproduce the problem with an specific scene, so I can test if the problem happens again

Another update, this time fixing Cycles UDIM rendering and crashes in 3D texture painting with missing tiles.

While I did find an issue in the Cycles code, I couldn't reproduce memory leaks or the issue described by @Alex Fuller (mistaed) and @Juan Gea (juang3d). If you could send me a file and/or instructions to reproduce the problem, that'd be great :)

Also, just as a general note to be clear: As far as I can see, the code is ready for review. There are some missing things (workbench engine, packing, baking) but I don't consider these to be critical for an initial implementation.

@Lukas Stockner (lukasstockner97) I'm going to test again and if it fails again I'll try to prepare an example blend file that always crashes.

Keep in mind the crashes were under GPU only, not CPU :)

Hey @Lukas Stockner (lukasstockner97) I've been able to test the patch again, and maybe the problem was mixed with something else, because I don't get the crash anymore, that's awesome :)

But I still get a problem that seems related to it since I only get this problem when I apply this patch, the preview sphere don't show the texture, it's like it's not finding it, but as you can see in the render, it finds the texture at render time (viewport or not)

Also another thing I noticed is with the Blender Cloud Addon, without this patch it works without problem, but with this patch I get this problem:

Since it appears just with this patch I thought it could be related.

I'm trying to reproduce the scene with the problem with a texture I can use publicly to share it.

@Lukas Stockner (lukasstockner97) With permission of Dorian, author of the new scatter plugin, where this texture is packed, I prepared a scene that shows the problem.

1.- As is said, it seems that there is no trouble anymore with the render giving an error, not in viewport or F12, so it could have been related to what Alex said, and it's now solved.

2.- But in the material preview the sphere is pink, it like the texture is not being detected, and it happens when I apply this patch, without the patch everythign works.

3.- The Blender Cloud addon error is also present only when I apply this patch, without the patch it works perfectly.

I hope this file helps finding the problem, I left the texture is packed inside.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 4 2019, 1:06 AM

Seems generally fine, also because we already did some review with an earlier version of this patch.

  • Image paint undo doesn't work correctly when painting on the second tile here. Paint in the second tile, then undo, it will modify the first tile.
  • For the UI, would it make sense to explicitly call things "UDIM Tiles" in the UI rather than just "Tiles". I think that might help people find the functionality, and disambiguates it with other types of tiles. Maybe same for the API properties, unless we expect to support different types of systems in the future?
intern/cycles/kernel/svm/svm_image.h
74

I think this is ok for now, but for making this work in more cases I think it will make more sense to store the UDIM information per image texture rather than per image texture node node, maybe in TextureInfo.

intern/cycles/render/graph.h
323 ↗(On Diff #19918)

Is this parameter really needed? It's not a big deal, but would be nice if shader graphs can potentially reusable by different shader.

As far as I can tell, we could pass the shader graph to simplify_settings and then instead of this, search for it being equal to shader->graph.

if (std::find(mesh->used_shaders.begin(), mesh->used_shaders.end(), shader) !=
        mesh->used_shaders.end()) {
intern/cycles/render/image.cpp
633

ToDo -> TODO (for consistency)

intern/cycles/render/nodes.cpp
319–325

Only run this code for UDIM images.

Especially since time complexity of this is quite poor :O(num_image_nodes x num_meshes)

Some comments could be left regarding further optimization here. Caching of the UV tiles so it's only computed once for multiple image nodes, avoiding the poor time complexity and avoiding the lookup entirely for the case when we have a texture cache system in the future.

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

Code convention: always use {}, and (ima == NULL)

source/blender/blenkernel/intern/image_save.c
400

Use BKE_imageuser_default()

source/blender/draw/intern/draw_manager_data.c
1218

Use BKE_imageuser_default to initialize this.

source/blender/editors/sculpt_paint/paint_image_2d.c
102

This doesn't seem to be used for anything?

112–114

Maybe rename EMPYT to UNINITIALIZED, and FAILED to EMPTY_OR_FAILED?

At least from what I understand that more accurately describes the meaning.

122

Maybe rename uv_ofs to uv_origin? It's a bit more clear to me. A comment explaining what this is would also help.

920

Can this be renamed to something like paint_2d_tile_init_canvas? check is kind of vague.

1627

Always use {}.

1698

Use BKE_imageuser_default, and maybe do it for the whole array?

1902–1912

Could this code be moved into a separate function and shared with gradient fill? Something like:

/* Acquire image buffer for tile that initial mouse position was over. */
ImBuf *paint_2d_bucket_acquire_ibuf(Image *ima, float image_init[2], float uv_ofs[2])
{
  int tile_number = BKE_image_get_tile_from_pos(ima, image_init, image_init, uv_ofs);

  ImageUser *iuser = &s->tiles[0].iuser;
  for (int i = 0; i < s->num_tiles; i++) {
    if (s->tiles[i].iuser.tile == tile_number) {
      if (!paint_2d_check_tile(s, i))
        return NULL;
      iuser = &s->tiles[i].iuser;
      break;
    }
  }

  ibuf = BKE_image_acquire_ibuf(ima, iuser, NULL);
}
source/blender/editors/sculpt_paint/paint_image_proj.c
745–747

Use BKE_imageuser_default

4298

Use BKE_imageuser_default

source/blender/editors/space_image/image_draw.c
666–685

Could be in a separate function, or should at least get a quick comment explaining what this does.

814

Would a more accurate name be draw_udim_tile_grids? Since it doesn't draw the actual images.

866

Unnecessary

source/blender/editors/space_image/image_ops.c
1272

Should this check only the filename, not the entire filepath?

source/blender/makesrna/intern/rna_image.c
250–259

I think sizeof(tile->label) will work here to replace both hardcoded 64 values.

839

I find the conversion between zero based and UDIM tile numbers rather confusing. Here I guess the API function expects zero based, while Image.tile_number uses UDIM encoding.

Also internally in Blender code there are a bunch of conversions. Is there anything stopping us from always using UDIM encoding? Or always using zero based, and then having a separate ImageTile.udim_tile_number for covenience and UI display?

851

This breaks API compatibility and there are various add-ons using it. Other properties like size remain available, presumably returning the size of the first tile. So can't this just return the bindcode of the first tile too?

source/blender/nodes/shader/nodes/node_shader_tex_image.c
134

It might be good to add a comment here explain we use this somewhat inefficient method because OpenGL texture arrays require all textures to have the same size, and bindless textures are not supported everywhere yet.

This revision now requires changes to proceed.Dec 4 2019, 1:06 AM

@Lukas Stockner (lukasstockner97), I was rather late reviewing this, but do you plan to include this for 2.82 still? Deadline is December 12 currently. I think this could go in and maybe get some further refinements while in master.

The only thing is, you'll have to be responsible for committing and handling potential issues that come up after, since I'll be on vacation without much internet between 13-25 December.

First of all, thanks for the review!

I'm currently working on addressing the feedback, I hope to have an updated version tomorrow.

I was quite busy for the last while but it's better now, so I think I'll be able to deal with any issues that come up.

Lukas Stockner (lukasstockner97) marked 22 inline comments as done.

Addressed all review comments and updated to latest master.

Note that since this version changes the internal format, older versions of the patch will not be able to open newer files anymore.
There is temporary versioning for the opposite (older files in this version), but I think we should remove that for the final commit (just open and save all older files with this first in order for them to work with official builds once this is merged).

Also, this fixed an issue in the versioning code and the preview issue found by @Juan Gea (juang3d).

Apparently updating via arc doesn't include inline comments, so here they are.

intern/cycles/kernel/svm/svm_image.h
74

Agreed, I'll just leave this as is for now but add a comment.

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

Good point, I overlooked this quite a few times. I also went with if (pointer != NULL) instead of if (pointer) for now, not sure whether that is required but I guess it never hurts to make it clear.

source/blender/draw/intern/draw_manager_data.c
1218

Ah, nice, good to know that exists.

I spotted a few other places that don't use this, I replaced the ones I saw but there probably are more, just for the future.

Also, side note: A lot of code is doing iuser->ok = true even though ok is an integer. Afaics that is fine since the conversion is guaranteed to result in 1, but I still really don't like that since ok can have values other than 0 or 1.

source/blender/editors/sculpt_paint/paint_image_2d.c
102

I forgot to initialize cache in BrushPainterCache, thanks for spotting that.

112–114

Good point, I went with MISSING for the second one for now.

920

I went with paint_2d_ensure_tile_canvas for now since it's always called, not just for initializing it, and I saw a similar naming in a few other places.

source/blender/editors/space_image/image_ops.c
1272

Good point, thanks.

Also, side note: Due to using BLI_stringdec, the current code will only work if the 1001 is the last number in the filename, so something like Diffuse_1001_Object_15.png will not work. I think that's fine, just wanted to point it out.

source/blender/makesrna/intern/rna_image.c
839

Good point, I didn't like the "external number" vs. "internal number" thing either.

I thought about it some more and there indeed is nothing stopping us from using the 1001-based indices everywhere. It requires a bit of logic to handle legacy code asking for tile 0, but that's fine (and would even allow us to handle that differently in the future).

851

This can easily be added back, yes.

Thanks for the update. Please be sure to have some basic release notes when this is committed, documenting limitations and basic usage.

This revision is now accepted and ready to land.Dec 12 2019, 8:34 AM

Also, please create a task with the remaining limitations to solve and add it to T66305: Render & Cycles Module..

Task is created: T72390
I added it to T66305, but I think it also belongs to other modules? Maybe I should split it up into subtasks and add those to the module tasks?

Sorry, submitted the comment too early: I can't add it to T66305 since I don't have edit permissions there.

I added you to the Moderators project, I think you should be able to edit it now.

I think it's fine to have the one task in the Render & Cycles module, main thing is that it is listed somewhere. But if you want to split it up that's convenient too.