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

Merge SetUniformXYZ methods into one with switch by type #5

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

mks-m
Copy link
Collaborator

@mks-m mks-m commented Apr 7, 2018

No description provided.

@mks-m mks-m self-assigned this Apr 7, 2018
@mks-m mks-m requested a review from maxfish April 7, 2018 10:40
Copy link
Owner

@maxfish maxfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try like this since it's cleaner and then we'll check the performances

@@ -118,24 +118,28 @@ func (s *ShaderProgram) GetUniform(name string) int32 {
return uniform
}

func (s *ShaderProgram) SetUniform4f(name string, value [4]float32) {
func (s *ShaderProgram) SetUniform(name string, val interface{}) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the performances of this method. It will be called several times per primitive per frame.
Uniform1fv for one single float might be slower than Uniform1f (we should benchmark it), for the other I think the compiler might be able to select the right switch without runtime penalty

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i think this is really micro-optimizing it. this should be good enough for vast majority of cases. but i will run some benchmarks but type switch is on level of nanoseconds: https://stackoverflow.com/questions/28024884/does-a-type-assertion-type-switch-have-bad-performance-is-slow-in-go

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was only about the single float32 case: uniform1fv vs uniform1f
I don't think there will be much difference and, anyway, we can always swap them.
Good to go!

@mks-m mks-m merged commit fa237ed into master Apr 9, 2018
@maxfish maxfish deleted the set-uniform-by-type branch April 18, 2018 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants