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

[release-caspian] Fix binder.setUniform error symbol in draw loop #9491

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented Apr 2, 2020

Fixes #9468 .

This regression was caused as an unintended sideeffect of d85afad.

The Uniform caching in that change works as expected, except in the symbol layer wherein the Program instance is only initialized once for all tiles in the draw loop.

Since the tiles need a round trip from the worker when certain paint properties are updated, for a few frames there can be mix of old and new tiles on screen, prior to the optimization the uniform bindings would be re-evaluated every frame, so it would self-heal. But the new implementation requires that the instance of ProgramConfiguration passed to Program#constructor and Program#draw be the same.

Launch Checklist

  • briefly describe the changes in this PR
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fixes a bug in which an exception would get thrown when updating symbol layer paint property using setPaintProperty</changelog>

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Nice catch! Good to merge for the quick fix, but I wonder if this could have unintended performance consequences since initializing the program/size once for all tiles was a performance optimization in the first place, as far as I remember.

So maybe a gentler fix would be to simply check for binder.setUniform before calling it in ProgramConfiguration, which would theoretically fix the error for these brief frames without having to change the drawing code.

@arindam1993
Copy link
Contributor Author

Every other layer does it this way tho, I ran some profiles on my macbook and nothing was really showing up there. Also, the caching of size also seems logically incorrect because it could get initialized from a text bucket, but apply the value to icon sizes later all in the draw loop when it encounters an icon bucket.

So maybe a gentler fix would be to simply check for binder.setUniform before calling it in ProgramConfiguration, which would theoretically fix the error for these brief frames without having to change the drawing code.

Wouldn't that fix also have potential sideffects wherein the shader has a uniform but then the binders have an attribute array?.

@sgolbabaei is actually working on a refactor of the relationship between ProgramConfiguration, Program and Bucket as a part of #9384. I think she's already got it to the point wherein ProgramConfiguration doesnt depend on layoutAttributes anymore, so. it can be created on the main thread now, without any tile data. That should make it much easier to have a simpler ( and maybe more performant ?)mapping between Programs and ProgramConfigurations without all the loop-de-loop that there is right now.

Because right now, ProgramConfigurations are created in the worker in a Bucket, even though all its state is largely dependent on the style layer and its props, and then its passed into the Program constructor, and then re-passed into the Program#draw, while Program is also caching metadata it received from ProgramConfiguration back when it was first created.

@mourner
Copy link
Member

mourner commented Apr 2, 2020

@arindam1993 found the original optimization I recalled — #3499 — apparently at that point it was only setting state once on program change, but that got refactored out at some point or another, likely after introducing program state tracking. So the change should be good after all, and I'm really looking forward to @sgolbabaei's refactoring — currently the relation is indeed pretty confusing (I'm still struggling with fully understanding how it all works even after all these years).

@arindam1993 arindam1993 merged commit 3c335ba into release-caspian Apr 2, 2020
@arindam1993 arindam1993 deleted the uniforms-fix-caspian branch April 2, 2020 18:31
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 this pull request may close these issues.

2 participants