Details

Reviewers

Brecht Van Lommel (brecht)
Lukas Stockner (lukasstockner97)
Campbell Barton (campbellbarton)

Maniphest Tasks

T74425: Can't texture paint an images sequence anymore

Commits

rB2bf4c74130ff: Fix T74425: Cannot texture paint an images sequence anymore
rBca717f0489af: Fix T74425: Cannot texture paint an images sequence anymore

Summary

Caused by the introduction of UDIM (rBc30d6571bb47).

We need to make sure the tiles ImageUser is set up correctly [especially the framenr], otherwise BKE_image_acquire_ibuf() and friends will fail to find the correct ImBuf.

Also instead of initializing a minimal BKE_imageuser_default, now use an appropriate ImageUser if avaliable and pass this around (instead of just the tile_number).
2D painting can reuse the Image Editor ImageUser, for 3D painting we still rely on a default ImageUser in most places, but at least set the framenr correctly].

This also fixes crashes when doing image operations such as inverting or resizing on images in a sequence in the Image Editor.
This also fixes color sampling (S) from the 3DView going wrong for image sequences (would fallback to OpenGL sampling because an ImBuf could not be found).

Diff Detail

Repository: rB Blender

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Mar 4 2020, 2:49 PM

Harbormaster completed remote builds in B6938: Diff 22370.Mar 4 2020, 2:49 PM

I don't think this is the ideal solution. Rather than initializing a default image user, we should set the appropriate image user.

For paint_image_2d.c, that means the image user of SpaceImage.

For image_undo.c, that would mean passing along the ImageUser that probably already exists from wherever it is called, instead of only the tile number.

This revision now requires changes to proceed.Mar 4 2020, 5:56 PM

Review by brecht:
Instead of initializing a minimal BKE_imageuser_default, now use an appropriate ImageUser if avaliable and pass this around (instead of just the tile_number).
Unfortunately, this involved more changes than anticipated, passing the ImageUser around (spawning into all the way into ProjPaintImage & PaintTile structs)... not sure if this is really needed [compared to the first version of this patch...]

Harbormaster completed remote builds in B6962: Diff 22421.Mar 5 2020, 4:00 PM

remove comment

Harbormaster completed remote builds in B6963: Diff 22422.Mar 5 2020, 4:01 PM

Philipp Oeser (lichtwerk) added a reviewer: Campbell Barton (campbellbarton).Mar 5 2020, 4:06 PM

It's more code, but also more correct.

source/blender/editors/sculpt_paint/paint_image_proj.c
201	Don't turn this into a pointer.
source/blender/editors/space_image/image_undo.c
110–114	Just use `ImageUser iuser;`.
532	Don't turn this into a pointer.
671–682	This can all be replaced by simply doing `uh->iuser = *iuser;`, copying the whole struct. But still set `ok = 1` as before. I'm not sure that's strictly needed, but safest to just keep it.

This revision now requires changes to proceed.Mar 5 2020, 4:37 PM

review by brecht

Harbormaster completed remote builds in B6971: Diff 22432.Mar 5 2020, 6:02 PM

Brecht Van Lommel (brecht) accepted this revision.Mar 5 2020, 6:04 PM

This revision is now accepted and ready to land.Mar 5 2020, 6:04 PM

note: need to get rid of a BKE_imageuser_default usage in projection paint as well (otherwise painting image sequences in the 3DView can still go wrong), will update in a bit...

fixes for projection (3D) painting, color sampling

Harbormaster completed remote builds in B6984: Diff 22456.Mar 6 2020, 11:36 AM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)Mar 6 2020, 11:40 AM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

Philipp Oeser (lichtwerk) marked 4 inline comments as done.

With regards to the undo system, would prefer not to have the ImageUser, which stores the scene, and various other settings we don't need.
If someone actually tries to use the ImageUser as it's intended (passing to API's that use it for example), it's going to cause problems because most of the values are cleared in this case.

Also, this structure is often associated with loading an image, reading code that takes an Image and an ImageUser, I assume it will need to load the image from disk - in some situation where the ImBuf may have been freed.
However that's not the case for the undo-system.

It looks like we only need the tile_number and frame, so those could be wrapped into a new struct and passed to ED_imapaint_dirty_region, ED_image_paint_tile_push, ED_image_paint_tile_find and stored in the image buffers undo steps.

This revision now requires changes to proceed.Mar 6 2020, 1:26 PM

It looks like we only need the tile_number and frame, so those could be wrapped into a new struct and passed to ED_imapaint_dirty_region, ED_image_paint_tile_push, ED_image_paint_tile_find and stored in the image buffers undo steps.

It's not only frames and tiles. Also the pass, layer and stereo eye index.

With regards to the undo system, would prefer not to have the ImageUser, which stores the scene, and various other settings we don't need.
If someone actually tries to use the ImageUser as it's intended (passing to API's that use it for example), it's going to cause problems because most of the values are cleared in this case.

The scene is only used for viewer and render results, and painting those should not be possible. But if we did support it, the scene would be need to distinguish the render buffers of different scenes.

I'm not sure which other values would be cleared?

The point of this change is exactly that you get the same ImBuf from the Image if you pass the same ImageUser.

Also, this structure is often associated with loading an image, reading code that takes an Image and an ImageUser, I assume it will need to load the image from disk - in some situation where the ImBuf may have been freed.
However that's not the case for the undo-system.

If the user can paint on ImBufs that can get freed after which the undo no longer works, that's a problem. But I don't see how that would get any worse with this change. At least now you can see that rather than trying to apply the undo step to the wrong frame or layer.

It's not only frames and tiles. Also the pass, layer and stereo eye index.

My concern is that we use an ImageUser that stores a bunch of values that aren't used or don't work properly.

If they can be used and set correctly, then there are no problems.

Just checked and painting onto passes isn't working, although it could be made to and the undo-system could support it if the ImageUser is stored.

Looking into this in more detail, if the ImageUser is always used from the SpaceImage for 2D painting, and constructed from defaults with assigned tile & frame for 3D-projection painting,
then this should be fine.

What I'd like to avoid is mixing up these fake ImageUser and image-user from the image-space, which may set all the values.

Campbell Barton (campbellbarton) accepted this revision.Mar 7 2020, 4:46 AM

This revision is now accepted and ready to land.Mar 7 2020, 4:46 AM

Campbell Barton (campbellbarton) added inline comments.Mar 7 2020, 4:56 AM

source/blender/editors/space_image/image_undo.c
110–114	This could use a comment that gives some details about it's use: For 2D image painting this uses most of the values. Even though views and passes are stored they are currently not supported for painting. For 3D projection painting this only uses a tile & frame number. The scene pointer must be cleared (or temporarily set it as needed, but leave cleared).

Campbell Barton (campbellbarton) added inline comments.Mar 9 2020, 2:50 AM

source/blender/editors/space_image/image_undo.c
669	Either assert scene is NULL, or clear it after assignment.

Brecht Van Lommel (brecht) accepted this revision.Mar 9 2020, 4:52 PM

FYI: there is still something wrong with projection painting and tiles now [noticed in udim-monster.blend], investigating...

review by @Campbell Barton (campbellbarton) (clear iuser scene pointer, add notes/assert about this)
dont use iuser pointer for PrepareImageEntry

Harbormaster completed remote builds in B7062: Diff 22617.Mar 10 2020, 1:13 PM

Thx @Campbell Barton (campbellbarton) , @Brecht Van Lommel (brecht) holding my hand here.
Would kindly ask on a last check regarding the last changes (not using a pointer for PrepareImageEntry)

Brecht Van Lommel (brecht) accepted this revision.Mar 10 2020, 1:24 PM

Closed by commit rBca717f0489af: Fix T74425: Cannot texture paint an images sequence anymore (authored by Philipp Oeser (lichtwerk)). · Explain WhyMar 10 2020, 1:54 PM

This revision was automatically updated to reflect the committed changes.

Philipp Oeser (lichtwerk) added a commit: rBca717f0489af: Fix T74425: Cannot texture paint an images sequence anymore.

Nathan Letwory (jesterking) added a commit: rB2bf4c74130ff: Fix T74425: Cannot texture paint an images sequence anymore.Mar 10 2020, 2:07 PM

Philipp Oeser (lichtwerk) mentioned this in D7435: Fix assert for Image Editor invert/resize operators.Apr 15 2020, 1:11 PM

Philipp Oeser (lichtwerk) mentioned this in rBf250b1f5a007: Fix assert for Image Editor invert/resize operators.Apr 15 2020, 5:46 PM