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

Fix DistanceFieldVector on WebGL iOS #374

Closed

Conversation

@fgoujeon
Copy link
Contributor

fgoujeon commented Sep 28, 2019

Hi mosra,

I've finally found a fix for this issue: mosra/magnum-examples#65 !
This commit fixes all the errors and warnings of ShadersDistanceFieldVectorGLTest.

For some reason, iOS doesn't like that texture binding to 15. I've set it to 0, like in Flat. Is there any reason why it wasn't set to 0 in the first place?

Also, I've replaced /2.0 with *0.5 so as to fix that Overflow in implicit constant conversion, minimum range for lowp float is (-2,2) warning without altering float precision.

I haven't tried to build the text example with this fix, but I can tell you it works for my project as it's now finally able to render everything correctly :).

@fgoujeon fgoujeon force-pushed the fgoujeon:fix-distancefieldvector-on-webgl-ios branch from c4e9c79 to 22cafcd Sep 28, 2019
@mosra mosra added this to the 2019.0b milestone Sep 28, 2019
@mosra mosra added this to TODO in GL via automation Sep 28, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 28, 2019

You're amazing, thanks a lot!

I remember fixing a similar issue in 2e4beb3, but of course wasn't dilligent enough to go through all other shaders and check those have low-enough binding points as well. And quite a lot time passed since then, so when you reported mosra/magnum-examples#65, I didn't even realize it could be similar.

It wasn't zero because usually you have a single distance field texture bound for e.g. text rendering all the time, while texture binding 0 is used for diffuse textures for all models etc. But I don't have a strict policy on that either.

Will check other shaders to be sure this mistake is not repeated yet again somewhere else and then merge this. Thank you again.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 2, 2019

Hi, so this got merged in 837defa (I added a bunch of comments here and there, hope that's okay), and to prevent such issues from reappearing in the future (or at least to make them easier to fix), I extended the shader tests for all builtin shaders in a515bdf. Just to have everything verified, would you have a chance to run all these (DistanceFieldVectorGLTest, VectorGLTest, ... in the Shaders/Test dir) on your setup and give me the output? Thank you! 🙏

Re the issue you mentioned in mosra/magnum-examples#65 -- building from master requires native Corrade (and corrade-rc, in particular) to be fresh from master as well. There were some incompatible changes to it lately that cause the exact issue you described.

@mosra mosra moved this from TODO to In Progress in GL Oct 2, 2019
@fgoujeon

This comment has been minimized.

Copy link
Contributor Author

fgoujeon commented Oct 2, 2019

Hi mosra,

I added a bunch of comments here and there, hope that's okay

It's perfectly ok :).

would you have a chance to run all these [tests] on your setup and give me the output?

Unfortunately the iPhone I used last week wasn't mine and I had to give it back to its owner. I'll see if I can find someone to run the tests on their iPhone.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 3, 2019

the iPhone I used last week wasn't mine

Haha, that perfectly describes how I'm doing iOS/macOS testing myself as well :) I'll keep this open until you (or me (or someone)) can try those out. Considering your patches were integrated and I'm not aware of any issues currently reported for the other shaders on any platforms, I'd assume the tests would pass.

@mosra mosra modified the milestones: 2019.0b, 2019.0c Oct 15, 2019
@mosra mosra closed this Oct 15, 2019
GL automation moved this from In Progress to Done Oct 15, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor

Melix19 commented Oct 16, 2019

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 16, 2019

Thanks a lot! In c632099 I inflated the test thresholds to accept those minor differences. The only thing left is that Phong reports a bunch of warning about lowp not being enough for doing *2.0 or /float(LIGHT_COUNT). Not sure what's the proper way to handle these, in both cases it's operations on values coming out of a 8-bit-per-channel texture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GL
  
Done
3 participants
You can’t perform that action at this time.