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

v1.9.0 add "fill-extrusion-pattern" webgl is not working properly #9479

Closed
nuancheng opened this issue Mar 30, 2020 · 8 comments
Closed

v1.9.0 add "fill-extrusion-pattern" webgl is not working properly #9479

nuancheng opened this issue Mar 30, 2020 · 8 comments
Assignees
Labels

Comments

@nuancheng
Copy link

The latest version adds 3D architectural texture, and the webgl is not working properly
V1.8.0 is normal

mapbox-gl-js version: v1.9.0

browser: Google Chrome 80.0.3987.149

Steps to Trigger Behavior

  1. v1.9.0 "vector" layer add "fill-extrusion-pattern" img

Link to Demonstration

"fill-extrusion-pattern": { property: "height", type: "interval", stops: [[40, "catcat"]] },

wrong:https://jsbin.com/yazutumoxa/edit?html,output
Expectant:https://jsbin.com/yazutumoxa/3/edit?html,output

Expected Behavior

Actual Behavior

@nuancheng
Copy link
Author

@mourner
Copy link
Member

mourner commented Mar 30, 2020

@karimnaaji might be related to the change in #9372?

@karimnaaji karimnaaji self-assigned this Mar 30, 2020
@karimnaaji
Copy link
Contributor

karimnaaji commented Mar 30, 2020

I checked the sanity of #9372 to see if it introduced a regression. There are no regression introduced by this specific PR, though, while looking at that I found another bug, which is in the vertex layout of pattern attributes.

Explicitly setting this offset to 0 assumes that all vertices within this layout start at the same byte in the vertex format. This is incorrect since this layout has 4 different components, each of which starts at byte offsets 0, 8, 16 and 17. This may have been silently broken for some time and adding new attributes in this layout did not take effect for DDS.

For this particular issue, @nuancheng , you need to use addImage with {pixelRatio: 2} option since you are loading a @2x image manually, the difference is that in v1.8.0 we were using the device pixel ratio, which happened to match this 2x on a 2x screen. So this example will actually accidentally work under a 2x screen in v1.8.0. But, on my 1x screen this image pixel ratio is downgraded to a 1x incorrectly in v1.8.0:

Screen Shot 2020-03-30 at 4 28 46 PM

Using addImage with {pixelRatio: 2} will work as long as you are not using data driven expressions in fill-extrusion-pattern; for DDS that's a good thing you brought this issue up, I'll be submitting a PR very soon with a fix and render test coverage around it.

@nuancheng
Copy link
Author

As if we are not talking about the same problem, I mean the error of v1.9.0 shown here
1585621972664
But it only appears on Mac OS. It looks normal on windows
11230

@karimnaaji
Copy link
Contributor

karimnaaji commented Mar 31, 2020

We are talking about the same problem and I understand what you are trying to show, I tried to give you details so you can follow us there. The path forward would be taken from #9482.

Can you give more information on these new screenshots? The code is truncated and I'm wondering whether you used the same as the initial examples as I can't repro the first screenshot; it makes it hard to make any assumption about how it's being used.

Here is your code snippet with the fix: https://jsbin.com/dagixugazi/edit?html,output

@Plantain
Copy link

Plantain commented Mar 31, 2020

Possibly related as suggested in #9394 , previously zooming would keep scaling of fill-patterns relatively constant, now it gives visually distorted/broken behaviour, see an example of zooming below with:
'fill-pattern': ["concat", "bs_", ["to-string",["get", "idx"]]],
where idx varies across a few different patterns (the area shown is one pattern though).

Behaviour now, before and after zoom:
77984167-52712e00-736d-11ea-927e-e3fb02281290
77984162-51400100-736d-11ea-9361-b047e5405bc7

Behaviour previously, before and after zoom:
77984375-e17e4600-736d-11ea-9830-7acf9352729e
77984373-dfb48280-736d-11ea-9080-620932ef329c

@nuancheng
Copy link
Author

https://jsbin.com/dagixugazi/edit?html,output
The problem has been solved in this version. Thank you.

In addition, I want to ask you a question. Now after the building is pasted with texture, the magnified texture of the field of vision will be recalculated. Is there any way to prohibit recalculation?

15 <zoom< 16 :
image

16< zoom < 17 :
image

The texture effect varies greatly under different zoom, which makes people feel a little strange.

@karimnaaji
Copy link
Contributor

karimnaaji commented Apr 1, 2020

@Plantain Thanks for the details, using 'fill-pattern': ["concat", "bs_", ["to-string",["get", "idx"]]] will lead to the same code path and should be fixed by #9482. Make sure to use the appropriate pixel ratio when using addImage, if you happened to be using it.

@nuancheng I think we will need a better example, using the full atlas of icons was ok to exemplify the issue you initially opened but does not give an appropriate representation for what you are suggesting. Can you open another issue applying a texture that would seamlessly repeat to better express this?

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

No branches or pull requests

4 participants