Texture Paint: more Editor syncing
Changes PlannedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Sep 17 2021, 3:34 PM.

Details

Reviewers

Campbell Barton (campbellbarton)
Julien Kaspar (JulienKaspar)
Daniel Bystedt (dbystedt)

Maniphest Tasks

T88788: When selected a texture in the Properties panel shows another texture in the 3D Viewport.

Summary

This one adresses the remaining comments by @Julien Kaspar (JulienKaspar) in D11498,
namely:

sync to the active texture paint slot when changing image in the Image Editor
sync to the active image node when changing image in the Image Editor
do all syncing from this patch & D11497 & D11498 to the texture paint canvas (aka. Texture Slot "Single Image" mode - as opposed to "Material" mode) as well. (this could be split in a separate commit)

This also has the benefit of all materials (and objects) using the
texture we are currently painting actually updating in the viewport when
changing the texture in any of the editors.

ref T88788
ref D11496
ref D11497
ref D11498

Diff Detail

Repository

rB Blender

Branch

T88788_d (branched from master)

Build Status

Buildable 17223
Build 17223: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Sep 17 2021, 3:34 PM

Philipp Oeser (lichtwerk) created this revision.

Harbormaster completed remote builds in B17098: Diff 41987.Sep 17 2021, 3:34 PM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)Sep 17 2021, 3:34 PM

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

Silas Opel (Schamph) awarded a token.Sep 17 2021, 3:44 PM

Bulat (BulatKR) awarded a token.Sep 17 2021, 3:47 PM

Philipp Oeser (lichtwerk) added a project: Sculpt, Paint & Texture.Sep 17 2021, 3:50 PM

This has multiple loops over all materials, potentially performing depsgraph updates for materials that aren't used by the object.

Unless there is a *very* good reason to operate on all materials, I think it would be better to limit this to materials used by the object/object-data (see BKE_object_material_get),
to avoid excessive updates and setting the active node for materials that are unrelated as far as the user is concerned.
It's possible scanning all materials node-trees is a heavy operation too.

Besides this, only minor issues noted.

source/blender/blenkernel/BKE_material.h
101	`struct Image` should be declared outside parameter list.
source/blender/makesrna/intern/rna_sculpt_paint.c
518	picky no need for `struct`.
source/blender/makesrna/intern/rna_space.c
1780	In general calling `DEG_id_tag_update(..., 0)` with zero should be avoided, is there a more specific flag that can be used.
1789	remove printf before committing.
1798	Should this break?

This revision now requires changes to proceed.Sep 22 2021, 10:43 AM

address minor review comments
early continue

Harbormaster completed remote builds in B17223: Diff 42226.Sep 22 2021, 12:08 PM

Philipp Oeser (lichtwerk) marked 5 inline comments as done.Sep 22 2021, 12:11 PM

In D12540#328869, @Campbell Barton (campbellbarton) wrote:

This has multiple loops over all materials, potentially performing depsgraph updates for materials that aren't used by the object.

Unless there is a *very* good reason to operate on all materials, I think it would be better to limit this to materials used by the object/object-data (see BKE_object_material_get),
to avoid excessive updates and setting the active node for materials that are unrelated as far as the user is concerned.
It's possible scanning all materials node-trees is a heavy operation too.

As mentioned in chat, this is in order to see all consequences of the painting (as in: update al materials possibly using this texture), but agree if this turns out to be overkill, it could reduced to just material on the active object (I did check this on a rather large project though with quite a few textures where it was not noticable -- but sure there are projects out there which might have a magnitude of materials/textures/nodes/nodetrees in there)

I am going to try a couple of things.

In D12540#328917, @Philipp Oeser (lichtwerk) wrote:

In D12540#328869, @Campbell Barton (campbellbarton) wrote:

This has multiple loops over all materials, potentially performing depsgraph updates for materials that aren't used by the object.

Unless there is a *very* good reason to operate on all materials, I think it would be better to limit this to materials used by the object/object-data (see BKE_object_material_get),
to avoid excessive updates and setting the active node for materials that are unrelated as far as the user is concerned.
It's possible scanning all materials node-trees is a heavy operation too.

As mentioned in chat, this is in order to see all consequences of the painting (as in: update al materials possibly using this texture), but agree if this turns out to be overkill, it could reduced to just material on the active object (I did check this on a rather large project though with quite a few textures where it was not noticable -- but sure there are projects out there which might have a magnitude of materials/textures/nodes/nodetrees in there)

Sorry, forgot to mention that this is really only relevant if you are looking at this in solid shading mode and Color type set to Texture.
And since our default texture painting workspace only enforces Color type Texture on the active object (not all of them) it could mean that either:

having this update on the active object is enough [use BKE_object_material_get]
somehow check if there are viewports in solid mode with color type Texture and only do the expensive stuff then.

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).Sep 22 2021, 2:27 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/makesrna/intern/rna_space.c
1772	When always setting the paint canvas like this, you might end up with a render result or reference image set as the canvas.

Philipp Oeser (lichtwerk) added inline comments.Sep 22 2021, 3:43 PM

source/blender/makesrna/intern/rna_space.c
1772	You can already select a RenderResult as a canvas "by hand" in the toolsettings `Texture Slots` panel (but yes, should be avoided when switching images in the Image Editor -- or avoided more generally, see T73182 / D11179)

Brecht Van Lommel (brecht) added inline comments.Sep 22 2021, 7:37 PM

source/blender/makesrna/intern/rna_space.c
1772	The gereral case I'm concerned about is where you have an image editor open, with images that have no relation to any materials. That may be a render result, but also a reference image for example. Activating an existing image texture node, with an image that was already selected as a texture seems reasonable to me. But not images that are not part of any material.

silex (silex) awarded a token.Sep 22 2021, 9:43 PM

Philipp Oeser (lichtwerk) added inline comments.Sep 23 2021, 9:40 AM

source/blender/makesrna/intern/rna_space.c
1772	with images that have no relation to any materials This is what `Single Image` (as opposed to `Material` mode) in texture slots is for, but I 100% agree that Image Editor should stay out of the way in regards to syncing (at least when the image is not used in the material) in that case.