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

[Blend/generic dsl] enabled/disabled indexed #7

Open
elect86 opened this issue Mar 27, 2019 · 17 comments
Open

[Blend/generic dsl] enabled/disabled indexed #7

elect86 opened this issue Mar 27, 2019 · 17 comments

Comments

@elect86
Copy link
Contributor

elect86 commented Mar 27, 2019

fun getEnabled(index: Int): Boolean = GL30C.glIsEnabledi(GL30C.GL_BLEND, index)
fun setEnabled(index: Int, enabled: Boolean /*also `= true`?*/) = when {
    enabled -> GL30C.glEnablei(GL30C.GL_BLEND, index)
    else -> GL30C.glDisablei(GL30C.GL_BLEND, index)
}

or

fun isEnabled(index: Int): Boolean = GL30C.glIsEnabledi(GL30C.GL_BLEND, index)
fun enable(index: Int) = GL30C.glEnablei(GL30C.GL_BLEND, index)
fun disable(index: Int) = GL30C.glDisablei(GL30C.GL_BLEND, index)
@Sylvyrfysh
Copy link
Collaborator

Second one is cleaner and takes less of a branching performance hit. Minimal, but might as well pull everything we can out of it

@Sylvyrfysh
Copy link
Collaborator

Also, inline classes for things that can be enabled/disabled?

@Sylvyrfysh
Copy link
Collaborator

Inline classes can implement interfaces so we could just make an Enablable interface and implement it where needed

@Wasabi375
Copy link
Contributor

I don't quite understand how inline classes help here. We are talking about global state after all.
Also talking about the branching performance hit. This would probably be zero as the JVM normally would just inline a short method like this at runtime. Since the branching performance is only relevant if this is called with either true or false the branch will be removed at runtime. And if you call the function with a variable argument you would need to add a branch either way. I think something like

fun enable(enable: Boolean = true, index: Int) = ...

is best.

@Sylvyrfysh
Copy link
Collaborator

Inline classes are meant to prevent the user from providing illegal arguments, nothing more. I'm no expert on the JVM, so if you think that method is better, I'll take your word for it.

@Wasabi375
Copy link
Contributor

Ok, I thought you wanted to represent the state of enabled/disabled in an inline class somehow, that makes more sense.
As for the JVM I'm not an expert either, but I thought this was one of the most common optimizations the JVM does. Maybe we should just take the save way and provide 2 functions. I don't think there are many cases where you want to call enable/disable dynamically. I at least just draw in 2 batches (first all opaque and then after that I sort and draw the rest with alpha).

@Wasabi375
Copy link
Contributor

One think we should look into is the difference between glEnablei and glEnable for blending.
As far as I understand glEnablei just sets the blending for a single drawbuffer while glEnable sets it for all. Not sure how glISEnabled behaves after the indexed set is called. Anyway we should provide access to both versions.

@Sylvyrfysh
Copy link
Collaborator

Burkhard is correct on the differences of glEnable and glEnablei, so that should be added in

@elect86
Copy link
Contributor Author

elect86 commented Mar 27, 2019

do you guys prefer explicit glEnablei instead of simply glEnable(index: Int)?

@Wasabi375 following that style proposal then the whole shall be

fun isEnabled(index: Int): Boolean = GL30C.glIsEnabledi(GL30C.GL_BLEND, index)
fun enable(index: Int, enable: Boolean = true) = when {
    enabled -> GL30C.glEnablei(GL30C.GL_BLEND, index)
    else -> GL30C.glDisablei(GL30C.GL_BLEND, index)
}

it's always best when default args are at the end..

@Sylvyrfysh
Copy link
Collaborator

I would like to have both. If I have 4 framebuffer and want to enable something for all of them, I don't want to make 4 enable calls, but rather the global one. Buf if I only want to enable it for only one, I'd rather have the index one. Maybe enable and enableIndex

@elect86
Copy link
Contributor Author

elect86 commented Mar 27, 2019

No wait, I think you misunderstood. We have here glIsEnabled, glEnable and glDisable and their indexed counterparts with the i postfix.

Shall we keep the postfix or just drop it because it's automatically when you pass index?

@Wasabi375
Copy link
Contributor

I'm not sure what I prefer. If we drop the i people might assume (if they are new to ogl) that enable() is the same as enable(0) since it looks like a default parameter. On the other hand the trailing i does look strange in a language where it's not required.
Maybe we could think of a better name like enableForDrawbuffer(index: Int). It's a bit longer but it clearly states what it does. And since we create wrappers around glEnable and glEnablei we are not required to have a completely generic name.
Also based on this we do not really need to worry about glEnablei for much more than blending since only GL_SCISSOR_TEST and GL_BLEND are allowed for the indexed version (with an index > 0).

@elect86
Copy link
Contributor Author

elect86 commented Mar 28, 2019

There are a couple of consideations to make:

  • we shall identify the target this library is meant for. I always assumed one should have already a (at least) basic knowledge of opengl. This is also the case of similar libraries in the C/C++ world (here and here)

  • I totally skipped (for the moment) all the set functions postfixes, such as f, iu, etc.. Everything is simply inferred from what is passed: if you set something passing an Int, you want to set it as an Int, if you pass a Float, you want to save it as a Float, and so on.
    I must admit this is more a type matter though, since the i in glEnablei is more logic-wise.
    I find expanding it a nice idea, but enableForDrawbuffer does sound too much specific. What about enableIndexed? OpenGL has used the -indexed postfix extensively in the later gl version (3.0+). It may be a nice way to armonize the semantic..

@Wasabi375
Copy link
Contributor

I think just enable is good for now. I just added the idea of enableForDrawbuffer as a different way of thinking about it. Also it was meant to be only used in a specific context. I think we had a dsl for blend/clear settings. I wouldn't use enableForDrawbuffer(capability: Enum, index: Int) since the "drawbuffer" part is blending specific. The index is used differently for GL_SCISSOR_TEST I think. I have not really used that before.
And I guess I agree with you that this is meant for people who have some basick knowledge of opengl.
If you don't you wouldn't be able to use this library anyway as we don't provide a magic function which sets everything up perfectly 😉

@Sylvyrfysh
Copy link
Collaborator

I agree with Burkhard's last point. We can't make people who don't know OpenGL use things correctly and so it should not be up to us to clarify the usages. Those who know will look and understand

@elect86
Copy link
Contributor Author

elect86 commented Mar 29, 2019

Ok, then we leave like it is for the moment

@elect86
Copy link
Contributor Author

elect86 commented Mar 29, 2019

Another thing.

Given that all the enable/disable capabilities are also available through vars, ie:

gl.blend = true

We might also extend it for those two indexed capabilities.. proposals:

  • gl.drawBuffer[0].blend = true
  • gl.viewport[1].scissor(Test) = false

or

  • gl.blendDrawBuffer[0] = true
  • gl.scissor(Test)Viewport[1] = false

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

No branches or pull requests

3 participants