Associate WebGL textures with texture units #18580

MortimerGoro · 2017-09-20T20:30:51Z

Currently bound_texture_2d and bound_texture_cube_map fields are used to restore texture states and to get the current bindings in GetParams(...). But as soon as active texture is changed all the values can become dirty, leading to broken textures in some demos or invalid getParam(...) call results.

This PR implements the texture binding association with the texture units.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-09-20T20:30:55Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webglrenderingcontext.rs
@fitzgen: components/script/dom/webglrenderingcontext.rs
@emilio: components/script/dom/webglrenderingcontext.rs

highfive · 2017-09-20T20:30:56Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

bors-servo · 2017-09-26T09:30:32Z

☔ The latest upstream changes (presumably #18635) made this pull request unmergeable. Please resolve the merge conflicts.

jdm

Looking for!

jdm · 2017-09-27T15:44:32Z

components/script/dom/webglrenderingcontext.rs

@@ -1938,12 +2001,32 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
    }

    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
+    #[allow(unrooted_must_root)]


Why is this necessary?

Because of the dictionary iterator()

Expression of type std::collections::hash_map::Iter<'_, u32, dom::webglrenderingcontext::TextureUnitBindings> must be rooted'''

Is there any better way to avoid that?

Add a new entry to https://github.com/servo/servo/blob/master/components/script_plugins/unrooted_must_root.rs#L57.

jdm · 2017-09-27T15:44:32Z

components/script/dom/webglrenderingcontext.rs

+    };
+}
+
+macro_rules! bound_texture {


Rather than a macro, I think a bound_texture method that accepts an enum argument would be more readable.

jdm · 2017-09-28T15:51:57Z

components/script/dom/webglrenderingcontext.rs

-            TexImageTarget::CubeMapNegativeY |
-            TexImageTarget::CubeMapPositiveZ |
-            TexImageTarget::CubeMapNegativeZ => self.bound_texture_cube_map.get(),
+    fn bound_texture(&self, target: u32) -> Option<DomRoot<WebGLTexture>> {


If we take a custom enum argument that only accepts Texture2d and TextureCubeMap, we don't need to return an Option. This will simplify other calling code, and then we can remove the optional_root_object_tojs_or_null macro.

Ok, I'll create the new enum. The result will be an Option too because the bound texture is nullable.

Oh, I see. In that case I don't think there's any strong benefit to using a separate enum.

jdm · 2017-09-28T15:51:57Z

components/script/dom/webglrenderingcontext.rs

@@ -1938,12 +2001,32 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
    }

    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
+    #[allow(unrooted_must_root)]


Add a new entry to https://github.com/servo/servo/blob/master/components/script_plugins/unrooted_must_root.rs#L57.


        Associate bound WebGLTextures with texture units.

MortimerGoro · 2017-09-28T18:21:19Z

Changes made, thanks for the unrooted_must_root info!

jdm · 2017-09-28T19:12:02Z

@bors-servo: r+

bors-servo · 2017-09-28T19:12:03Z

📌 Commit b9710a5 has been approved by jdm

bors-servo · 2017-09-28T19:12:09Z

⌛ Testing commit b9710a5 with merge ffa0338...


        Auto merge of #18580 - MortimerGoro:webgl_active_texture, r=jdm

Associate WebGL textures with texture units  Currently `bound_texture_2d` and `bound_texture_cube_map` fields are used to restore texture states and to get the current bindings in GetParams(...). But as soon as active texture is changed all the values can become dirty, leading to broken textures in some demos or invalid getParam(...) call results. This PR implements the texture binding association with the texture units. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18580)

bors-servo · 2017-09-28T20:10:51Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing ffa0338 to master...

MortimerGoro requested a review from emilio Sep 20, 2017

highfive assigned jdm Sep 20, 2017

highfive added the S-awaiting-review label Sep 20, 2017

highfive added the S-needs-rebase label Sep 26, 2017

jdm requested changes Sep 27, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 27, 2017

MortimerGoro force-pushed the MortimerGoro:webgl_active_texture branch from b992375 to 918169a Sep 28, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 28, 2017

jdm requested changes Sep 28, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Sep 28, 2017

Associate bound WebGLTextures with texture units.

Loading status checks…

b9710a5

MortimerGoro force-pushed the MortimerGoro:webgl_active_texture branch from 918169a to b9710a5 Sep 28, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 28, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 28, 2017

jdm approved these changes Sep 28, 2017

View changes

bors-servo merged commit b9710a5 into servo:master Sep 28, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 28, 2017

servo / servo

Associate WebGL textures with texture units #18580

Associate WebGL textures with texture units #18580

MortimerGoro commented Sep 20, 2017 •

edited

highfive commented Sep 20, 2017

highfive commented Sep 20, 2017

bors-servo commented Sep 26, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Sep 28, 2017

jdm commented Sep 28, 2017

bors-servo commented Sep 28, 2017

bors-servo commented Sep 28, 2017

bors-servo commented Sep 28, 2017

servo / servo

Join GitHub today

Associate WebGL textures with texture units #18580

Associate WebGL textures with texture units #18580

Conversation

MortimerGoro commented Sep 20, 2017 • edited

highfive commented Sep 20, 2017

highfive commented Sep 20, 2017

bors-servo commented Sep 26, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Sep 28, 2017

jdm commented Sep 28, 2017

bors-servo commented Sep 28, 2017

bors-servo commented Sep 28, 2017

bors-servo commented Sep 28, 2017

MortimerGoro commented Sep 20, 2017 •

edited