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

New Issue

Christian Stolze · 2020-12-29T01:13:15+01:00

Christian Stolze commented

2020-12-29 01:13:15 +01:00

First-time contributor

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):

**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)

Christian Stolze commented

2020-12-29 01:13:15 +01:00

First-time contributor

Added subscriber: @regcs

Christian Stolze commented

2020-12-29 01:17:01 +01:00

First-time contributor

Added subscriber: @Jeroen-Bakker

Jacob Merrill commented

2020-12-29 06:16:46 +01:00

First-time contributor

Added subscriber: @JacobMerrill-1

Robert Guetzkow commented

2020-12-29 10:46:52 +01:00

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

Gottfried Hofmann commented

2021-01-05 14:17:46 +01:00

First-time contributor

Added subscriber: @GottfriedHofmann

Andy Jarosz commented

2021-04-20 19:00:16 +02:00

First-time contributor

Added subscriber: @Andyfilms-1

Andy Jarosz commented

2021-04-20 19:00:16 +02:00

First-time contributor

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.

martin fröhlich commented

2021-04-23 16:19:02 +02:00

First-time contributor

Added subscriber: @MartinFrohlich

martin fröhlich commented

2021-04-23 16:19:02 +02:00

First-time contributor

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.

Gottfried Hofmann commented

2021-06-18 13:09:11 +02:00

First-time contributor

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

Aaron Carlisle commented

2021-08-05 03:16:56 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Aaron Carlisle closed this issue

2021-08-05 03:16:56 +02:00

Aaron Carlisle self-assigned this 2021-08-05 03:16:56 +02:00

Christian Stolze commented

2021-08-29 18:19:02 +02:00

First-time contributor

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?

Aaron Carlisle commented

2021-08-30 17:10:04 +02:00

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

Christian Stolze commented

2021-08-31 11:34:11 +02:00

First-time contributor

@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.

Aaron Carlisle commented

2021-09-04 03:35:38 +02:00

I would leave it up to @Jeroen-Bakker

Christian Stolze commented

2021-09-22 16:17:17 +02:00

First-time contributor

Added subscriber: @mont29

Christian Stolze commented

2021-09-22 16:17:18 +02:00

First-time contributor

@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.

Christian Stolze commented

2021-09-22 16:37:46 +02:00

First-time contributor

Added subscriber: @ideasman42

Bastien Montagne commented

2021-09-22 17:06:22 +02:00

Added subscriber: @dfelinto

Bastien Montagne commented

2021-09-22 17:06:22 +02:00

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.

Christian Stolze commented

2021-09-22 17:16:07 +02:00

First-time contributor

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.

Dalai Felinto commented

2021-09-23 14:37:49 +02:00

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).

Christian Stolze commented

2021-09-23 18:45:06 +02:00

First-time contributor

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?

Campbell Barton commented

2021-09-24 07:47:23 +02:00

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).

Christian Stolze commented

2021-09-29 21:35:06 +02:00

First-time contributor

@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!

Dalai Felinto commented

2021-09-30 10:41:55 +02:00

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.