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

Use active uniforms data to implement gl.uniform* checks #21184

Merged
merged 4 commits into from Jul 17, 2018

Conversation

Projects
None yet
5 participants
@nox
Copy link
Member

commented Jul 16, 2018

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 16, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/webglprogram.rs, components/script/dom/webgluniformlocation.rs
  • @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/webglprogram.rs, components/script/dom/webgluniformlocation.rs
@highfive

This comment has been minimized.

Copy link

commented Jul 16, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm
Copy link
Member

left a comment

This looks good, but there's also a lot of duplication of checks. It would be good to file a followup issue to figure out way to reduce the duplication, similar to https://searchfox.org/mozilla-central/source/dom/canvas/WebGLContextGL.cpp#1964-2061.

) {
self.with_location(location, |location| {
match location.type_() {
constants::FLOAT_VEC2 => {}

This comment has been minimized.

Copy link
@jdm

jdm Jul 16, 2018

Member

I believe we want to keep handling booleans in other methods as well - https://www.khronos.org/registry/OpenGL-Refpages/es3.0/ says:

Either the i, ui or f variants may be used to provide values for uniform variables of type bool, bvec2, bvec3, bvec4,
or arrays of these. The uniform variable will be set to false if the input value is 0 or 0.0f, and it will be set to true
otherwise.

@jdm jdm assigned jdm and unassigned asajeffrey Jul 16, 2018

@jdm

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

error: unused import: `WebGLResult`
 --> components/script/dom/webgluniformlocation.rs:6:44
  |
6 | use canvas_traits::webgl::{WebGLProgramId, WebGLResult};
  |                                            ^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
error: aborting due to previous error
error: Could not compile `script`.
@nox

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

I reduced the duplication a tad bit but I'll file an issue later about that.

I also fixed the various boolean uniforms that were missing.

@nox

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

📌 Commit 1c585c3 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

⌛️ Testing commit 1c585c3 with merge 5217314...

bors-servo added a commit that referenced this pull request Jul 16, 2018

Auto merge of #21184 - servo:webgl, r=jdm
 Use active uniforms data to implement gl.uniform* checks

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21184)
<!-- Reviewable:end -->

Fixed the boolean stuff and will file an issue for the duplication.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

  ▶ CRASH [expected OK] /_mozilla/webgl/conformance-1.0.3/conformance/ogles/GL/gl_FragCoord/gl_FragCoord_001_to_003.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 18.1.0-devel
  │ DESCRIPTION: WebGL GLSL conformance test: gl_FragCoord_001_to_003.html
  │ 
  │ test: gl_FragCoord_xy_frag.test.html
  │ loading: ../default/default.vert
  │ loading: gl_FragCoord_xy_frag_ref.frag
  │ loading: ../default/default.vert
  │ loading: gl_FragCoord_xy_frag.frag
  │ loaded: gl_FragCoord_xy_frag_ref.frag
  │ loaded: ../default/default.vert
  │ 
  │ loaded: ../default/default.vert
  │ loaded: gl_FragCoord_xy_frag.frag
  │ 
  │ PASS images are the same
  │ PASS getError was expected value: NO_ERROR : there should be no errors
  │ 
  │ test: gl_FragCoord_z_frag.test.html
  │ loading: gl_FragCoord_z_frag_ref.vert
  │ loading: gl_FragCoord_z_frag_ref.frag
  │ loading: ../default/default.vert
  │ loading: gl_FragCoord_z_frag.frag
  │ loaded: gl_FragCoord_z_frag_ref.vert
  │ loaded: gl_FragCoord_z_frag_ref.frag
  │ 
  │ assertion failed: location >= 0 (thread WebGLThread, at components/canvas/webgl_thread.rs:1120)
  │ stack backtrace:
  │    0:     0x7fd2ef19241c - backtrace::backtrace::trace::h48eb2921f133e389
  │    1:     0x7fd2ef192252 - <backtrace::capture::Backtrace as core::default::Default>::default::h00deb8ea10633b85
  │    2:     0x7fd2ef192298 - backtrace::capture::Backtrace::new::h6d1048ca147e297d
  │    3:     0x7fd2ec4e2728 - servo::main::{{closure}}::h343b2e747a03b7e2
  │    4:     0x7fd2ef1bbad3 - std::panicking::rust_panic_with_hook::hf121af3526fe4125
  │                         at libstd/panicking.rs:479
  │    5:     0x7fd2ee00a476 - std::panicking::begin_panic::h06fa6a45cd442aff
  │    6:     0x7fd2ee0206ad - canvas::webgl_thread::WebGLImpl::apply::haa471dfb2c5427b9
  │    7:     0x7fd2ee01cc48 - <canvas::webgl_thread::WebGLThread<VR, OB>>::handle_msg::h4a10c052a0820ae4
  │    8:     0x7fd2ee009e25 - std::sys_common::backtrace::__rust_begin_short_backtrace::h817ad4c5b5af86ee
  │    9:     0x7fd2ee00a4d8 - _ZN3std9panicking3try7do_call17hd4ba12f5477611fcE.llvm.15623415505575742723
  │   10:     0x7fd2ef1cd499 - __rust_maybe_catch_panic
  │                         at libpanic_unwind/lib.rs:105
  │   11:     0x7fd2ee0120f6 - <F as alloc::boxed::FnBox<A>>::call_box::h67a22db4f56cc3f6
  │   12:     0x7fd2ef1a66fa - <alloc::boxed::Box<(dyn alloc::boxed::FnBox<A, Output$u3d$R$GT$$u20$$u2b$$u20$$u27$a$RP$$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::haf969131982fb97c
  │                         at /checkout/src/liballoc/boxed.rs:650
  │                          - std::sys_common::thread::start_thread::hcbfc448db5968f29
  │                         at libstd/sys_common/thread.rs:24
  │   13:     0x7fd2ef1a4985 - std::sys::unix::thread::Thread::new::thread_start::ha4b2330dc32d1193
  │                         at libstd/sys/unix/thread.rs:90
  │   14:     0x7fd2eb3bd183 - start_thread
  │   15:     0x7fd2e9c8403c - clone
  │   16:                0x0 - <unknown>

@nox nox force-pushed the webgl branch from 1c585c3 to 476640c Jul 16, 2018

@nox

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

Needed to check for gl_ prefix.

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

📌 Commit 476640c has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

⌛️ Testing commit 476640c with merge 3003e6f...

bors-servo added a commit that referenced this pull request Jul 16, 2018

Auto merge of #21184 - servo:webgl, r=jdm
 Use active uniforms data to implement gl.uniform* checks

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21184)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

@bors-servo bors-servo merged commit 476640c into master Jul 17, 2018

3 of 5 checks passed

Tidelift An error occured
Details
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.