Skip to content

Remove redundant check for state.atlasTextureIcon in drawLayerSymbols - #10038

Closed
ghost wants to merge 1 commit into
mainfrom
unknown repository
Closed

Remove redundant check for state.atlasTextureIcon in drawLayerSymbols#10038
ghost wants to merge 1 commit into
mainfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Oct 17, 2020

Copy link
Copy Markdown

This removes a redundant check in drawLayerSymbols.
We already check for state.atlasTextureIcon in line 367, and (unless I'm missing something) it shouldn't have changed by line 369.


(It would be nice if you can tag this PR for hacktoberfest inclusion as described in https://hacktoberfest.digitalocean.com/details#rules - feel free to ignore this last bit if you don't want this)

@ryanhamley

Copy link
Copy Markdown
Contributor

This double check was added in #8904 specifically https://github.com/mapbox/mapbox-gl-js/pull/8904/files#diff-32491b8d513a9d3eb4a65cd5b5ded4b70be4f2cea814adbc54f437eaccf1af0cR367-R372 so I don't believe it's an oversight. Changing the activeTexture may update state.atlasTextureIcon. @alexshalamov can you confirm that?

@karimnaaji karimnaaji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch!

Changing the activeTexture may update state.atlasTextureIcon.

@ryanhamley there's no chance for that, it will only set the active texture binder in the webgl context, no side effect involved.

Comment thread src/render/draw_symbol.js
state.atlasTexture.bind(state.atlasInterpolation, gl.CLAMP_TO_EDGE);
if (state.atlasTextureIcon) {
context.activeTexture.set(gl.TEXTURE1);
if (state.atlasTextureIcon) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Was this meant to check atlasInterpolationIcon maybe?

@CLAassistant

CLAassistant commented Dec 8, 2020

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Jannik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ryanhamley

ryanhamley commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

@JannikGM If you'd still like to contribute this, you will need to sign our new Contributor License Agreement. I'm going to close this as stale, but if you sign the CLA, we can re-open it.

@ryanhamley ryanhamley closed this Jan 12, 2021
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.

3 participants