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

Add WebGL rendering tests & improve the WebGLHelper API #8977

Merged
merged 5 commits into from Nov 20, 2018

Conversation

jahow
Copy link
Contributor

@jahow jahow commented Nov 19, 2018

In this PR:

  • Two new rendering tests: heatmap-layer and webgl-points
  • Simpler shader compilation with the WebGLHelper class, the WebGLFragment and WebGLVertex classes were removed to simplify the system overall

Previously caches of shaders and programs were maintained using the objects uid which required these intermediary classes.

@jahow jahow force-pushed the add-webgl-rendering-tests branch 3 times, most recently from c0aa262 to b97fc57 Compare November 19, 2018 11:02
Now the shader and program caches are simply arrays of native WebGL created objects.
The WebGLHelper simply takes the sources of the frag and vert shader and produces a program.

This removes 2 classes & reduces the general verbosity of the API.

Also a `getShaderCompilationErrors` was added on `WebGLHelper` to help debug GLSL errors.
@jahow
Copy link
Contributor Author

jahow commented Nov 19, 2018

Damn, the webgl rendering on CircleCI is broken...

  • Expected:
    image

  • Actual:
    image

@jahow jahow changed the title Add WebGL rendering tests & improve the WebGLHelper API [WIP] Add WebGL rendering tests & improve the WebGLHelper API Nov 19, 2018
@jahow jahow changed the title [WIP] Add WebGL rendering tests & improve the WebGLHelper API Add WebGL rendering tests & improve the WebGLHelper API Nov 20, 2018
@jahow
Copy link
Contributor Author

jahow commented Nov 20, 2018

@tschaub @ahocevar the webgl rendering tests finally passed with the --ignore-gpu-blacklist option set for chrome. This PR is ready for merge, thanks for any review!

@tschaub tschaub merged commit c6be2c7 into openlayers:master Nov 20, 2018
@tschaub
Copy link
Member

tschaub commented Nov 20, 2018

Glad you were able to configure Puppeteer in a way that works in CI without complicating local dev. Thanks for the work on this @jahow.

@jahow
Copy link
Contributor Author

jahow commented Nov 20, 2018

I'm glad too ;) thanks

@ahocevar
Copy link
Member

@jahow I think something is still wrong with the rendering tests. When I run them locally (with npm run test-rendering), I get

uncaught exception [Error: TypeError: Cannot read property 'indexOf' of undefined
    at includes (http://127.0.0.1:3000/cases/heatmap-layer/main.js:7003:14)
    at new WebGLHelper (http://127.0.0.1:3000/cases/heatmap-layer/main.js:51843:93)
    at new WebGLPointsLayerRenderer (http://127.0.0.1:3000/cases/heatmap-layer/main.js:41100:20)
    at Heatmap.createRenderer (http://127.0.0.1:3000/cases/heatmap-layer/main.js:29191:12)
    at Heatmap.getRenderer (http://127.0.0.1:3000/cases/heatmap-layer/main.js:29693:29)
    at Heatmap.render (http://127.0.0.1:3000/cases/heatmap-layer/main.js:29630:32)
    at CompositeMapRenderer.renderFrame (http://127.0.0.1:3000/cases/heatmap-layer/main.js:37942:29)
    at Map.renderFrame_ (http://127.0.0.1:3000/cases/heatmap-layer/main.js:4559:20)
    at Map.<anonymous> (http://127.0.0.1:3000/cases/heatmap-layer/main.js:3525:12)]
case ./cases/heatmap-layer/main.js (Heatmap layer renders properly using webgl)': mismatch 0.738

The rendered result is a blank image. Maybe this is related to #9014?

@jahow
Copy link
Contributor Author

jahow commented Nov 30, 2018

I think removing this parameter should solve the problem :

const gl = getContext(canvas, {failIfMajorPerformanceCaveat: true});

This "indexof" error appears when the context creation did not succeed, which sometimes happen depending on the environment.

@ahocevar
Copy link
Member

@jahow Thanks for looking into this so quickly. Removing this parameter makes the exception go away indeed, but the test still fails with a mismatch of 0.148. Looking at the generated result, I see no heatmap at all. Just the base map. So I guess the remaining issue is still related to #9014.

@jahow
Copy link
Contributor Author

jahow commented Nov 30, 2018

Which I successfully reproduced on a MacOS environment so should be fixable.

Although I'm a bit worried about all the discrepancies between platforms with the webgl rendering. So far the GPU was blacklisted in the CI, I have major glitches on debian/firefox and the rendering fails altogether for a different reason on MacOS. I just hope it comes from my implementation... Did you have those sort of issues with the previous webgl renderers?

@ahocevar
Copy link
Member

The previous webgl renderers were also unreliable, but at least I got something to see. Now, it just renders nothing.

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.

None yet

3 participants