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

Show only 1 once the message "Parameter not found" #566

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@ZerpHmm
Contributor

ZerpHmm commented Apr 6, 2014

No description provided.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 6, 2014

Member

Would you mind describing the problem?

If I get it right, you insert the -1 value into the cache so that the next time, it will be recognized, and no further error message is emitted? Have you made sure that this doesn't break the semantics elsewhere?

Member

Bromeon commented Apr 6, 2014

Would you mind describing the problem?

If I get it right, you insert the -1 value into the cache so that the next time, it will be recognized, and no further error message is emitted? Have you made sure that this doesn't break the semantics elsewhere?

@criptych

This comment has been minimized.

Show comment
Hide comment
@criptych

criptych Apr 11, 2014

I've used a similar technique before. In order for a nonexistent (and in some implementations, unused) uniform to become available, the shader source would have to change; SFML already clears the parameter cache when that happens (in compile), so the location will have to be looked up again anyway.

criptych commented Apr 11, 2014

I've used a similar technique before. In order for a nonexistent (and in some implementations, unused) uniform to become available, the shader source would have to change; SFML already clears the parameter cache when that happens (in compile), so the location will have to be looked up again anyway.

@eXpl0it3r eXpl0it3r added the bug label May 13, 2014

@LaurentGomila LaurentGomila added confirmed and removed unassigned labels May 20, 2014

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 23, 2014

Member

All this PR does is make sure that the shader only has to look up a name->location mapping once per compiled program. The mappings are specific to a compiled & linked program and thus they are already deleted in the current implementation when doing something that causes the program to be recompiled.

This doesn't solve the issue of preventing warning messages when deliberately trying to set a parameter that doesn't exist in the shader, but at least it limits the warning to just the first attempt to set the parameter. This should be enough for the developer to fix unintended behaviour adequately.

In the future there should be a way to optionally set parameters of a shader if the developer knows that their shaders all specify different uniforms. It isn't even enough just specifying them and leaving them unused. If the shader compiler notices that after compiling/optimizing, a uniform isn't used anywhere, it won't show up in the location registry of the shader and you will get spammed with warnings by SFML.

I'd say just apply this PR for now. A more flexible parameter system will come some time in the future.

Member

binary1248 commented May 23, 2014

All this PR does is make sure that the shader only has to look up a name->location mapping once per compiled program. The mappings are specific to a compiled & linked program and thus they are already deleted in the current implementation when doing something that causes the program to be recompiled.

This doesn't solve the issue of preventing warning messages when deliberately trying to set a parameter that doesn't exist in the shader, but at least it limits the warning to just the first attempt to set the parameter. This should be enough for the developer to fix unintended behaviour adequately.

In the future there should be a way to optionally set parameters of a shader if the developer knows that their shaders all specify different uniforms. It isn't even enough just specifying them and leaving them unused. If the shader compiler notices that after compiling/optimizing, a uniform isn't used anywhere, it won't show up in the location registry of the shader and you will get spammed with warnings by SFML.

I'd say just apply this PR for now. A more flexible parameter system will come some time in the future.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

Note: this PR is not in sync with master and has to milestone set.

Member

mantognini commented May 26, 2014

Note: this PR is not in sync with master and has to milestone set.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jun 1, 2014

Contributor

I'd like to see this being merged, too!
It has annoyed me quiet a few times. Especially the case that @binary1248 has discribed, when something in the shader code is wrong and the variable gets optimized out. Any other (meaningful) warning/error message gets lost in the flooded console.

Contributor

Foaly commented Jun 1, 2014

I'd like to see this being merged, too!
It has annoyed me quiet a few times. Especially the case that @binary1248 has discribed, when something in the shader code is wrong and the variable gets optimized out. Any other (meaningful) warning/error message gets lost in the flooded console.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jun 1, 2014

Member

If I remember correctly, there are a few other places where a lot of error messages are emitted (mostly invalid GL calls). It could be worthwhile to have a look at them too.

Member

Bromeon commented Jun 1, 2014

If I remember correctly, there are a few other places where a lot of error messages are emitted (mostly invalid GL calls). It could be worthwhile to have a look at them too.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 4, 2014

Member

@ZerpHmm Would you mind updating your version to the latest master? Otherwise I'll create a new branch later on.

Member

eXpl0it3r commented Jun 4, 2014

@ZerpHmm Would you mind updating your version to the latest master? Otherwise I'll create a new branch later on.

@Bromeon Bromeon added the s:accepted label Jun 6, 2014

@TankOs TankOs added this to the 2.2 milestone Jun 9, 2014

@TankOs TankOs self-assigned this Jun 9, 2014

@TankOs TankOs added p:android and removed s:unassigned labels Jun 9, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs
Member

TankOs commented Jun 9, 2014

@TankOs TankOs removed p:android labels Jun 10, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 10, 2014

Member

Unless someone reviews this negatively, I'm going to merge into master.

Member

TankOs commented Jun 10, 2014

Unless someone reviews this negatively, I'm going to merge into master.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Jun 10, 2014

Member

Looks good to me.

Member

zsbzsb commented Jun 10, 2014

Looks good to me.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jun 10, 2014

Member

Has anybody tested the code yet?

Member

Bromeon commented Jun 10, 2014

Has anybody tested the code yet?

@TankOs TankOs removed the s:unassigned label Jun 11, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Yep, it does work flawlessly. Tested on Linux (behavior shouldn't be any different on other platforms).

Member

TankOs commented Jun 11, 2014

Yep, it does work flawlessly. Tested on Linux (behavior shouldn't be any different on other platforms).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jun 11, 2014

Member

I'd say go ahead and merge it.

Member

eXpl0it3r commented Jun 11, 2014

I'd say go ahead and merge it.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Merged. Thanks @ZerpHmm

Member

TankOs commented Jun 11, 2014

Merged. Thanks @ZerpHmm

@TankOs TankOs closed this Jun 11, 2014

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