Fix #103244: ghosting in Eevee with sculpt paint brush and canvas #104557

Open
Colin Marmond wants to merge 9 commits from Kdaf/blender:fix-103244-sculpt-lighting-using-paint-brush-on-image into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

When the user is drawing on an image with the paint brush in sculpt mode, the mutlisamping is creating ghosting (because of the optimisation of redrawing when only an image is modified).

To fix it (and improve perfs), I created a new region tag redraw function, forcing 1 sample redraw.

This fixes the issue of ghosting, and limit the number of samples to 1 during paint stokes.

PS : (maybe extend it to all sculpt brushes ? it could optimize a bit the redrawing)

When the user is drawing on an image with the paint brush in sculpt mode, the mutlisamping is creating ghosting (because of the optimisation of redrawing when only an image is modified). To fix it (and improve perfs), I created a new region tag redraw function, forcing 1 sample redraw. This fixes the issue of ghosting, and limit the number of samples to 1 during paint stokes. PS : (maybe extend it to all sculpt brushes ? it could optimize a bit the redrawing)
Colin Marmond added 1 commit 2023-02-10 09:29:33 +01:00
Colin Marmond changed title from {{Issue|103244}} Fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). to #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). 2023-02-10 09:35:20 +01:00
Jeroen Bakker requested review from Jeroen Bakker 2023-02-10 09:59:46 +01:00
Member

Do you have an example when this happens. It would make the PR more clear.
eg. with what setup is recalc shading needed, when only the texture pixels are changed. Recalc shading is an expensive process and can lead to performance penalties. For that reason there was an early exit.

Do you have an example when this happens. It would make the PR more clear. eg. with what setup is recalc shading needed, when only the texture pixels are changed. Recalc shading is an expensive process and can lead to performance penalties. For that reason there was an early exit.
Jeroen Bakker added this to the Sculpt, Paint & Texture project 2023-02-10 10:09:44 +01:00
Colin Marmond added 1 commit 2023-02-10 14:20:29 +01:00
Author
Member

I linked an example for illustration, and added a more specific condition for the shading redraw.

I'm sorry that this so little fix takes so much time, I'm also exploring the source code to get used to it in the future.

I linked an example for illustration, and added a more specific condition for the shading redraw. I'm sorry that this so little fix takes so much time, I'm also exploring the source code to get used to it in the future.
Colin Marmond added 1 commit 2023-02-10 14:35:04 +01:00
Jeroen Bakker reviewed 2023-02-10 15:26:09 +01:00
@ -5380,6 +5381,10 @@ void SCULPT_flush_update_step(bContext *C, SculptUpdateType update_flags)
if ((update_flags & SCULPT_UPDATE_IMAGE) != 0) {
ED_region_tag_redraw(region);
if (update_flags == SCULPT_UPDATE_IMAGE) {
if (ELEM(v3d->shading.type, OB_MATERIAL, OB_TEXTURE, OB_RENDER)) {
Member

I see what you're doing and why you have chosen this solution. I did have something else in mind to fix this, what doesn't need re-evaluating materials, shaders etc.

When the user is painting (Paint model operator is active) eevee should only use a single sample.
When the user finishes the stroke the rest of the samples can be calculated.

The ghosting effect appears as previous samples might have different colors. Blending multiple samples together leads to the ghosting. By just drawing a single sample we can make sure ghosting isn't working.

Although the final result is the same, I expect it to be lighter on the painting pipeline and improves the performance when using larger textures and complexer shaders.

We should prototype the other approach and see which one would be better in terms of performance and maintencance.

I see what you're doing and why you have chosen this solution. I did have something else in mind to fix this, what doesn't need re-evaluating materials, shaders etc. When the user is painting (Paint model operator is active) eevee should only use a single sample. When the user finishes the stroke the rest of the samples can be calculated. The ghosting effect appears as previous samples might have different colors. Blending multiple samples together leads to the ghosting. By just drawing a single sample we can make sure ghosting isn't working. Although the final result is the same, I expect it to be lighter on the painting pipeline and improves the performance when using larger textures and complexer shaders. We should prototype the other approach and see which one would be better in terms of performance and maintencance.
Kdaf marked this conversation as resolved
Author
Member

Oh, my bad, now I see the difference you are talking about. But I'm not sure how to start this prototype, as I do not have enough knowledge on this area.
Should I take the time, or is this patch needed in a short delay ?

Oh, my bad, now I see the difference you are talking about. But I'm not sure how to start this prototype, as I do not have enough knowledge on this area. Should I take the time, or is this patch needed in a short delay ?
Colin Marmond changed title from #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). to WIP: #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). 2023-02-10 20:11:57 +01:00
Colin Marmond added 3 commits 2023-02-11 08:32:57 +01:00
Colin Marmond added 1 commit 2023-02-11 08:36:36 +01:00
Author
Member

I have had some little problems with my git, but everything is now clean.
I probably did what you was thinking If that not the case, I'm so sorry.

I have had some little problems with my git, but everything is now clean. I probably did what you was thinking If that not the case, I'm so sorry.
Jeroen Bakker requested changes 2023-02-28 04:11:08 +01:00
Jeroen Bakker left a comment
Member

Idea is what is expected, but here is already a flag for painting check RV3D_PAINTING.
This should be set in RegionView3D.flags. This would make DRW_state_is_navigating
do the right thing already.

So for this task we should check if this flag will be set when performing sculpt texture painting.
BTW if you want a proper review, best to remove the 'WIP:' from the title.

Idea is what is expected, but here is already a flag for painting check `RV3D_PAINTING`. This should be set in `RegionView3D.flags`. This would make `DRW_state_is_navigating` do the right thing already. So for this task we should check if this flag will be set when performing sculpt texture painting. BTW if you want a proper review, best to remove the 'WIP:' from the title.
Colin Marmond added 1 commit 2023-02-28 08:13:09 +01:00
Colin Marmond added 1 commit 2023-02-28 09:17:47 +01:00
Colin Marmond changed title from WIP: #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). to #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). 2023-02-28 09:20:05 +01:00
Author
Member

I'm sorry for this messy PR, I'll do better in the next ones.

This should be fine now.

I'm sorry for this messy PR, I'll do better in the next ones. This should be fine now.
Colin Marmond requested review from Jeroen Bakker 2023-02-28 09:21:12 +01:00
Colin Marmond changed title from #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). to fix #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). 2023-02-28 09:21:32 +01:00
Jeroen Bakker reviewed 2023-02-28 11:37:41 +01:00
@ -253,8 +253,11 @@ int EEVEE_temporal_sampling_init(EEVEE_ViewLayerData *UNUSED(sldata), EEVEE_Data
const DRWContextState *draw_ctx = DRW_context_state_get();
const Scene *scene_eval = DEG_get_evaluated_scene(draw_ctx->depsgraph);
bool painting = false;
Member

This should be hidden in DRW_state_is_navigating or a similar function. Not sure yet if we need to split DRW_state_is_navigating... Best to ask direction in the eevee-viewport-module on blender.chat.

This should be hidden in `DRW_state_is_navigating` or a similar function. Not sure yet if we need to split DRW_state_is_navigating... Best to ask direction in the eevee-viewport-module on blender.chat.
Brecht Van Lommel changed title from fix #103244: fix shading in Eevee when using the canvas option in the paint brush (sculpt mode). to Fix #103244: ghosting in Eevee with sculpt paint brush and canvas 2023-03-10 13:29:50 +01:00
Jeroen Bakker requested changes 2023-04-03 08:10:24 +02:00
Jeroen Bakker left a comment
Member

This PR still needs some changes.

This PR still needs some changes.
Author
Member

Yes,

I would like to close this but as you mentionned the condition is to be placed in a safe place. I cant decide myself where it should be. If anyone can take just a quick look, it would be nice.

Yes, I would like to close this but as you mentionned the condition is to be placed in a safe place. I cant decide myself where it should be. If anyone can take just a quick look, it would be nice.
Julien Kaspar added the
Module
Sculpt, Paint & Texture
label 2023-05-01 10:42:45 +02:00
This pull request has changes conflicting with the target branch.
  • source/blender/draw/engines/eevee/eevee_temporal_sampling.c

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix-103244-sculpt-lighting-using-paint-brush-on-image:Kdaf-fix-103244-sculpt-lighting-using-paint-brush-on-image
git checkout Kdaf-fix-103244-sculpt-lighting-using-paint-brush-on-image
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104557
No description provided.