-
Notifications
You must be signed in to change notification settings - Fork 327
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
Add support for multisample FBOs and depth textures #1183
Conversation
Cleanup the code while I'm there.
If we grow this enum to support more color attachments or N depth attachments, we now have one place to change.
While this is a desktop GL command used for compat, the state is shared between all transformers in the stack, and without this logic the texture is never primed with a layer. Fixes compat issues.
Unify the framebuffer read-back logic for color and depth maps. Add support for multisample resolving. Use this logic for texture read-backs too.
|
||
func attachmentToEnum(a api.FramebufferAttachment) (GLenum, error) { | ||
switch a { | ||
case api.FramebufferAttachment_Color0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you know what although the enums are finite, the number of attachments is, in theory, unbounded. (implementation must support at least 4)
So ideally this should handle Depth and Stencil and do cast and arithmetic for the rest.
Alternatively what do you think of making FramebufferAttachment an integer instead? (where >= 0 is the respective colour, and negative values are used for depth/stencil)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, but these are declared in the protobuf. Negative enums in protos are discouraged.
I'll have a think about this, but this issue has not been made better or worse by this PR - opting to defer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about low two bits indicate type?
I am kind ok fine with the current approach, as well. But let's maybe add a few more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want more changes, let's do it in a follow up CL. This PR is urgent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since extending the Enum would require changes to the UI as well, that is probably best done later?
The other changes look fine to me.
@@ -435,21 +435,29 @@ public static PixelInfo compute(FloatBuffer buffer, boolean isRGBA) { | |||
if (isRGBA) { | |||
for (int i = 0, end = buffer.remaining() - 3; i <= end; ) { | |||
float value = buffer.get(i++); | |||
min = Math.min(min, value); | |||
max = Math.max(max, value); | |||
if (!Float.isNaN(value) && !Float.isInfinite(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
||
func attachmentToEnum(a api.FramebufferAttachment) (GLenum, error) { | ||
switch a { | ||
case api.FramebufferAttachment_Color0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since extending the Enum would require changes to the UI as well, that is probably best done later?
The other changes look fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me
Also handle
glDrawRangeElements
for the mesh view.