Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in 5.4.x related to off-screen render targets #233

Closed
toaarnio opened this issue Aug 1, 2018 · 7 comments · Fixed by #234
Closed

Regression in 5.4.x related to off-screen render targets #233

toaarnio opened this issue Aug 1, 2018 · 7 comments · Fixed by #234
Labels

Comments

@toaarnio
Copy link
Contributor

toaarnio commented Aug 1, 2018

Description

There is a regression related to off-screen render targets in 5.4.x. After using a 2-channel off-screen framebuffer, the main framebuffer starts acting like a 2-channel framebuffer as well. From then on, only the first two channels are affected by any operation, including clear().

Proof of Code

#!/usr/bin/python3 -B

import numpy as np          # pip install numpy
import moderngl as gl       # pip install moderngl
import pygame               # pip install pygame

def main():
    winsize = (4, 4)
    pygame.display.init()
    pygame.display.set_mode(winsize, pygame.OPENGL)
    ctx = gl.create_context()
    offscreen = ctx.simple_framebuffer(winsize, components=2, dtype='f1')
    offscreen.use()
    ctx.screen.use()
    ctx.clear(0.50, 0.50, 0.50, 1.0)
    pixels = ctx.screen.read(winsize, components=3, dtype='f1')
    pixels = np.frombuffer(pixels, dtype=np.uint8).reshape(4,4,3)
    print("EXPECTED: [128 128 128]")
    print("ACTUAL  :", pixels[0,0])
    assert(np.all(pixels[0,0] == [128, 128, 128]))

if __name__ == "__main__":
    main()

Result on 5.3.0:

$ ./moderngl-bug.py
EXPECTED: [128 128 128]
ACTUAL  : [128 128 128]

Result on 5.4.2:

$ ./moderngl-bug.py
EXPECTED: [128 128 128]
ACTUAL  : [128 128 0]
Traceback (most recent call last):
  File "./moderngl-bug.py", line 24, in <module>
    main()
  File "./moderngl-bug.py", line 20, in main
    assert(np.all(pixels[0,0] == [128, 128, 128]))
AssertionError

Version info

moderngl 5.4.2
--------------
vendor: Intel Open Source Technology Center
renderer: Mesa DRI Intel(R) HD Graphics 630 (Kaby Lake GT2) 
version: 4.5 (Core Profile) Mesa 18.0.5
python: 3.5.2 (default, Nov 23 2017, 16:37:01) 
[GCC 5.4.0 20160609]
platform: linux
code: 450
@szabolcsdombi
Copy link
Member

Please take a look at the following lines:

https://github.com/cprogrammer1994/ModernGL/blob/5.4.1/src/Context.cpp#L1392

https://github.com/cprogrammer1994/ModernGL/blob/5.4.1/src/Framebuffer.cpp#L379

We were changing these in the last releases.
I think we should restore the gl.DrawBuffers call, but the values should be properly filled by the Context when initialized.

This need investigation. I know that it was not properly implemented earlier and the gl.DrawBuffers set a gl error. according to docs gl.DrawBuffers can be called on the default framebuffer. The values should not contain any of the GL_COLOR_ATTACHMENTn s. I thought the gl.Get in the initialize returned the proper value, but probably not.

I will have time later to investigate it too.

@einarf
Copy link
Member

einarf commented Aug 2, 2018

Could be a good idea to add test(s) for this.

When I looked at the issue I saw the the default framebuffer (0) returning GL_BACK from gl.Get.

https://www.khronos.org/opengl/wiki/Default_Framebuffer might also be useful.

@toaarnio
Copy link
Contributor Author

toaarnio commented Aug 3, 2018

I tried adding my test case (see above) to the unit test suite, but the bug is only affecting mixed onscreen-offscreen rendering, and that's not supported in the test suite. In fact there are no onscreen test cases as far as I understand.

@einarf
Copy link
Member

einarf commented Aug 6, 2018

The output of your test code on OS X (nvidia 650M, Early 2013 macbook pro):

EXPECTED: [128 128 128]
ACTUAL  : [127 127   0]

@einarf
Copy link
Member

einarf commented Aug 6, 2018

Desktop Win10 nvidia 1070

EXPECTED: [128 128 128]
ACTUAL  : [127 127   0]

@toaarnio
Copy link
Contributor Author

toaarnio commented Aug 7, 2018

OK, I made a bad choice of multiplier because 255 * 0.5 equals 127.5, which can be rounded either way depending on the driver. Let's pick 0.4 instead, that'll give an exact result (102).

@szabolcsdombi
Copy link
Member

I will have time around the week end. We will fix it for sure. Sorry for the late response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants