[GPUOffscreen] draw_view3d doesn't apply color management settings #84227

Closed
opened 2020-12-29 01:13:15 +01:00 by Christian Stolze · 27 comments

System Information
Operating system: macOS Catalina (10.15.7)
Graphics card: Intel Iris Plus Graphics 1536 MB

Blender Version
Broken: 2.83.10, 2.90.0, 2.91.0
Worked: 2.82 (13-Feb-2020)

Short description of error

Rendering the viewport into an GPUOffscreen does not apply the color management of the current scene. This is most-likely related to e0ffb911a2, which was committed earlier to fix #74299. This commit states that color management was applied twice before the commit, now no color management seems to be applied at all.

Exact steps for others to reproduce the error

Run the example for offscreen rendering from:
https://docs.blender.org/api/master/gpu.html#rendering-the-3d-view-into-a-texture

Then change the color management settings and compare viewport and the drawn offscreen image.

import bpy
import bgl
import gpu
from gpu_extras.presets import draw_texture_2d

WIDTH = 512
HEIGHT = 256

offscreen = gpu.types.GPUOffScreen(WIDTH, HEIGHT)


def draw():
    context = bpy.context
    scene = context.scene

    view_matrix = scene.camera.matrix_world.inverted()

    projection_matrix = scene.camera.calc_matrix_camera(
        context.evaluated_depsgraph_get(), x=WIDTH, y=HEIGHT)

    offscreen.draw_view3d(
        scene,
        context.view_layer,
        context.space_data,
        context.region,
        view_matrix,
        projection_matrix)

    bgl.glDisable(bgl.GL_DEPTH_TEST)
    draw_texture_2d(offscreen.color_texture, (10, 10), WIDTH, HEIGHT)


bpy.types.SpaceView3D.draw_handler_add(draw, (), 'WINDOW', 'POST_PIXEL')

Example screenshot from 2.90.0 (2.90.0 2020-08-31):
draw_view3d.jpg

**System Information** Operating system: macOS Catalina (10.15.7) Graphics card: Intel Iris Plus Graphics 1536 MB **Blender Version** Broken: 2.83.10, 2.90.0, 2.91.0 Worked: 2.82 (13-Feb-2020) **Short description of error** Rendering the viewport into an GPUOffscreen does not apply the color management of the current scene. This is most-likely related to e0ffb911a2, which was committed earlier to fix #74299. This commit states that color management was applied twice before the commit, now no color management seems to be applied at all. **Exact steps for others to reproduce the error** Run the example for offscreen rendering from: https://docs.blender.org/api/master/gpu.html#rendering-the-3d-view-into-a-texture Then change the color management settings and compare viewport and the drawn offscreen image. ``` import bpy import bgl import gpu from gpu_extras.presets import draw_texture_2d WIDTH = 512 HEIGHT = 256 offscreen = gpu.types.GPUOffScreen(WIDTH, HEIGHT) def draw(): context = bpy.context scene = context.scene view_matrix = scene.camera.matrix_world.inverted() projection_matrix = scene.camera.calc_matrix_camera( context.evaluated_depsgraph_get(), x=WIDTH, y=HEIGHT) offscreen.draw_view3d( scene, context.view_layer, context.space_data, context.region, view_matrix, projection_matrix) bgl.glDisable(bgl.GL_DEPTH_TEST) draw_texture_2d(offscreen.color_texture, (10, 10), WIDTH, HEIGHT) bpy.types.SpaceView3D.draw_handler_add(draw, (), 'WINDOW', 'POST_PIXEL') ``` Example screenshot from 2.90.0 (2.90.0 2020-08-31): ![draw_view3d.jpg](https://archive.blender.org/developer/F9534736/draw_view3d.jpg)

Added subscriber: @regcs

Added subscriber: @regcs

Added subscriber: @Jeroen-Bakker

Added subscriber: @Jeroen-Bakker

Added subscriber: @JacobMerrill-1

Added subscriber: @JacobMerrill-1

Changed status from 'Needs Triage' to: 'Confirmed'

Changed status from 'Needs Triage' to: 'Confirmed'

Added subscriber: @GottfriedHofmann

Added subscriber: @GottfriedHofmann

Added subscriber: @Andyfilms-1

Added subscriber: @Andyfilms-1

I reverted back to 2.81 to see if this problem persisted there, and it actually is present there as well. It appears that this is either a rendering bug, or that code sample is incomplete in some way.

I reverted back to 2.81 to see if this problem persisted there, and it actually is present there as well. It appears that this is either a rendering bug, or that code sample is incomplete in some way.

Added subscriber: @MartinFrohlich

Added subscriber: @MartinFrohlich

This bug prevents users to use Blender for in-camera-effects in virtual production. Reason: with the https://github.com/maybites/blender.script.spout/ addon it is possible to render the texture to a video software to map it onto a LED screen or a projection.

This bug prevents users to use Blender for in-camera-effects in virtual production. Reason: with the https://github.com/maybites/blender.script.spout/ addon it is possible to render the texture to a video software to map it onto a LED screen or a projection.

Created a fix for this by exposing the color management option in the Python API while defaulting to false (the current behavior): https://developer.blender.org/D11645

Created a fix for this by exposing the color management option in the Python API while defaulting to false (the current behavior): https://developer.blender.org/D11645
Member

Changed status from 'Confirmed' to: 'Resolved'

Changed status from 'Confirmed' to: 'Resolved'
Aaron Carlisle self-assigned this 2021-08-05 03:16:56 +02:00

Is there a possibility that @GottfriedHofmann 's fix D11645 will also make it into Blender 2.93 LTS?

Is there a possibility that @GottfriedHofmann 's fix [D11645](https://archive.blender.org/developer/D11645) will also make it into Blender 2.93 LTS?
Member

Based on https://lists.blender.org/pipermail/bf-committers/2021-August/051126.html and that this is an API breaking change I do not believe this is a good candidate to be backported to 2.93 LTS

Based on https://lists.blender.org/pipermail/bf-committers/2021-August/051126.html and that this is an API breaking change I do not believe this is a good candidate to be backported to 2.93 LTS

@Blendify I was hoping that it could be backported to 2.93 LTS, because it is not API breaking. @GottfriedHofmann set the default behavior (i.e., the do_color_management argument is not specified) to false which makes the draw_view3d() method behave as it did for several Blender versions now.

@Blendify I was hoping that it could be backported to 2.93 LTS, because **it is not API breaking**. @GottfriedHofmann set the default behavior (i.e., the `do_color_management` argument is not specified) to `false` which makes the `draw_view3d()` method behave as it did for several Blender versions now.
Member

I would leave it up to @Jeroen-Bakker

I would leave it up to @Jeroen-Bakker

Added subscriber: @mont29

Added subscriber: @mont29

@mont29 @ideasman42 After talking to Jeroen on blender.chat, he suggested to ask you about your opinion. I believe, that this commit should be integrated into future patch versions of 2.93 LTS, since this Blender release will be here for the next two years. The commit is not API breaking, so older plugins will not be affected. On the other hand, newer add-ons get the opportunity to rely on this commit by Gottfried and support the LTS version as well.

@mont29 @ideasman42 After talking to Jeroen on blender.chat, he suggested to ask you about your opinion. I believe, that this commit should be integrated into future patch versions of 2.93 LTS, since this Blender release will be here for the next two years. The commit is not API breaking, so older plugins will not be affected. On the other hand, newer add-ons get the opportunity to rely on this commit by Gottfried and support the LTS version as well.

Added subscriber: @ideasman42

Added subscriber: @ideasman42

Added subscriber: @dfelinto

Added subscriber: @dfelinto

This is mostly a release management topic TBH...

Question being, do we accept that python code (add-ons or other) updated for LTS 2.93.n won't work in LTS 2.93.n-1?

Personnally I would rather say no, unless said API change is critical, which is not the case here afaics.

Wouldn't mind getting @dfelinto's feelings on this question though.

This is mostly a release management topic TBH... Question being, do we accept that python code (add-ons or other) updated for LTS 2.93.n won't work in LTS 2.93.n-1? Personnally I would rather say no, unless said API change is critical, which is not the case here afaics. Wouldn't mind getting @dfelinto's feelings on this question though.

But from an add-on developer perspective, I would say: We add-on developers have to take care for a proper version management of our add-ons. So, if I decide to use this specific new feature, then my add-on will check if the user is running 2.93.n or 2.93.n-1. If he is running an incompatible version, then the user will be informed. That is why we have the blender attribute in the bl_info of each add-on.

As long as the API change is not affecting old code (and in this case it doesn't, since it is an optional parameter that is introduced), it should be fine.

But from an add-on developer perspective, I would say: We add-on developers have to take care for a proper version management of our add-ons. So, if I decide to use this specific new feature, then my add-on will check if the user is running 2.93.n or 2.93.n-1. If he is running an incompatible version, then the user will be informed. That is why we have the `blender` attribute in the `bl_info` of each add-on. As long as the API change is not affecting old code (and in this case it doesn't, since it is an optional parameter that is introduced), it should be fine.

The API shouldn't change between the LTS unless extremely necessary.

In this case what I propose is to have offscreen buffer to always color manage. I can see some very special corner cases where color management is not required. In this case I would say:

  • In master (and if Jeroen agrees, LTS as well) we have CM on by default.
  • In master (for 3.0) we change the API to optionally turn off CM.

I know that effectively this changes the result of the buffers. But the fact that there was no CM in the first is a bug (in my original implementation).

The API shouldn't change between the LTS unless extremely necessary. In this case what I propose is to have offscreen buffer to always color manage. I can see some very special corner cases where color management is not required. In this case I would say: * In master (and if Jeroen agrees, LTS as well) we have CM on by default. * In master (for 3.0) we change the API to optionally turn off CM. I know that effectively this changes the result of the buffers. But the fact that there was no CM in the first is a bug (in my original implementation).

I still think, it is a (marginal) API change that would not affect anyone in a bad way. On the contrary, it fixes a bug with a maximum of compatibility - add-on developers using the do_management_color parameter would just need to take care of a proper version check.

Anyway, for my purpose it would also be fine to fix this bug like @dfelinto suggested: Switch on color management by default in 2.93 LTS and keep the patch by Gottfried only in 3.0 to enable others to turn the color management off, if required.

Would that be reasonable and okay for you, @mont29 and @Jeroen-Bakker?

I still think, it is a (marginal) API change that would not affect anyone in a bad way. On the contrary, it fixes a bug with a maximum of compatibility - add-on developers using the do_management_color parameter would just need to take care of a proper version check. Anyway, for my purpose it would also be fine to fix this bug like @dfelinto suggested: Switch on color management by default in 2.93 LTS and keep the patch by Gottfried only in 3.0 to enable others to turn the color management off, if required. Would that be reasonable and okay for you, @mont29 and @Jeroen-Bakker?

This seems like a reasonable change to back-port to the LTS, although in general I'd rather not backport any API changes, this is arguably a regression that can be worked around with an optional argument (so as not to change the behavior of existing scripts).

This seems like a reasonable change to back-port to the LTS, although in general I'd rather not backport any API changes, this is arguably a regression that can be worked around with an optional argument (so as not to change the behavior of existing scripts).

@ideasman42 I also would favor this solution. But I could also live with the option that @dfelinto suggested.

Thank you all for your input! I don't want to bother you all with that too much, but still want to push the decision a bit because the longer we wait the more versions of 2.93.x will be released without that change. And considering the concerns raised about diverging LTS versions here, I think it would make sense to implement the change as soon as possible.

Now the question is: Who makes the final decision? If I can assist in any way to speed up the process, I would be happy to do so!

@ideasman42 I also would favor this solution. But I could also live with the option that @dfelinto suggested. Thank you all for your input! I don't want to bother you all with that too much, but still want to push the decision a bit because the longer we wait the more versions of 2.93.x will be released without that change. And considering the concerns raised about diverging LTS versions here, I think it would make sense to implement the change as soon as possible. **Now the question is: Who makes the final decision?** If I can assist in any way to speed up the process, I would be happy to do so!

Looking a bit deeper into the problem I think it is simple to just port this to LTS. I added to the list of 2.93 commits to be ported.

Looking a bit deeper into the problem I think it is simple to just port this to LTS. I added to the list of 2.93 commits to be ported.

That's great, thank you @dfelinto!

That's great, thank you @dfelinto!
Sign in to join this conversation.
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 project
No Assignees
10 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#84227
No description provided.