Skip to content

/ servo

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

webgl: Track the current program, implement some uniform functions, and nits #9107

Merged
merged 8 commits into from Jan 13, 2016
Merged

webgl: Track the current program, implement some uniform functions, and nits #9107

merged 8 commits into from Jan 13, 2016

Conversation

@emilio
Copy link
Member

emilio commented Dec 31, 2015

Was done while implementing sequence arguments.

Depends on #9056.

Review on Reviewable
@emilio emilio force-pushed the emilio:webgl-uniforms-and-nits branch from 81e2e95 to 3a24119 Jan 4, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

☔ The latest upstream changes (presumably #8970) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio emilio force-pushed the emilio:webgl-uniforms-and-nits branch from 3a24119 to 38e9ab3 Jan 6, 2016
@jdm
Copy link
Member

jdm commented Jan 6, 2016

@simartin Since you've done some work in the WebGL code before, would you be interested in reviewing these changes?
@simartin
Copy link
Contributor

simartin commented Jan 9, 2016

Sure, I can take a look, thanks for suggesting :-)
@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2016

☔ The latest upstream changes (presumably #9201) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio emilio force-pushed the emilio:webgl-uniforms-and-nits branch from 38e9ab3 to 44690c2 Jan 10, 2016
@emilio emilio removed the S-needs-rebase label Jan 10, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2016

☔ The latest upstream changes (presumably #9255) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio emilio force-pushed the emilio:webgl-uniforms-and-nits branch 5 times, most recently from dd3e5ba to e030281 Jan 12, 2016
@emilio
Copy link
Member Author

emilio commented Jan 12, 2016

Now that #9056 has landed this is ready for review :P

r? @simartin
@simartin
Copy link
Contributor

simartin commented Jan 12, 2016

components/canvas/webgl_paint_thread.rs, line 160 [r3] (raw file):
I fail to understand why you remove Uniform4fv

Comments from the review on Reviewable.io
@emilio
Copy link
Member Author

emilio commented Jan 12, 2016

Uniform4fv([x, y, z, u]) is equivalent to Uniform4f(x, y, z, u), so I just defined the first in terms of the second.
@simartin
Copy link
Contributor

simartin commented Jan 12, 2016

components/canvas/webgl_paint_thread.rs, line 188 [r3] (raw file):
Wouldn't assert!(error != gl::NO_ERROR, "Unexpected WebGL error: {:x}", error); be more concise and as readable?

Comments from the review on Reviewable.io
@emilio
Copy link
Member Author

emilio commented Jan 12, 2016

Sure! Will change it ASAP (I'm on my phone right no, so no reviewable for me :/)
@simartin
Copy link
Contributor

simartin commented Jan 12, 2016

components/script/dom/webglrenderingcontext.rs, line 957 [r3] (raw file):
data.is_empty() maybe?

Comments from the review on Reviewable.io
@emilio
Copy link
Member Author

emilio commented Jan 12, 2016

Yup! Good catch :P
@simartin
Copy link
Contributor

simartin commented Jan 12, 2016

Apart from my minor comment, it looks good to me, thanks!

Comments from the review on Reviewable.io
@jdm
Copy link
Member

jdm commented Jan 12, 2016

@simartin Just so you know, you can make multiple comments in the reviewable interface and then click the publish button at the end to publish them all at the same time. That usually results in less email for other people :)
@KiChjang
Copy link
Member

KiChjang commented Jan 13, 2016

@ecoal95 Since you're a reviewer now, you can name anyone with r=[name]. You do not need to delegate first before you name them.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

⌛ Testing commit 449de16 with merge 3b49055...
bors-servo added a commit that referenced this pull request Jan 13, 2016
@bors-servo
Auto merge of #9107 - ecoal95:webgl-uniforms-and-nits, r=simartin
3b49055 
webgl: Track the current program, implement some uniform functions, and nits

Was done while implementing sequence arguments.

Depends on #9056.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

💔 Test failed - mac-rel-wpt
emilio added 3 commits Jan 4, 2016
@emilio
webgl: Improve debugging
48962bd 
Now we can keep track of errors more easily.
@emilio
webgl: Fix draw_arrays_simple reftests
cf479a2 
It used pre-shader translation syntax
@emilio
webgl: Check shader compilation status on use_program.
Loading status checks…
4092ffd
@emilio emilio force-pushed the emilio:webgl-uniforms-and-nits branch from 449de16 to 4092ffd Jan 13, 2016
@emilio
Copy link
Member Author

emilio commented Jan 13, 2016

@bors-servo: r=simartin (I'm stupid and I used the opposite assert condition, plus the travis build errored due to connection issues)
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

📌 Commit 4092ffd has been approved by simartin
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

⌛ Testing commit 4092ffd with merge 611e114...
bors-servo added a commit that referenced this pull request Jan 13, 2016
@bors-servo
Auto merge of #9107 - ecoal95:webgl-uniforms-and-nits, r=simartin
611e114 
webgl: Track the current program, implement some uniform functions, and nits

Was done while implementing sequence arguments.

Depends on #9056.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

💔 Test failed - linux-rel
@jdm
Copy link
Member

jdm commented Jan 13, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

⌛ Testing commit 4092ffd with merge c13e840...
bors-servo added a commit that referenced this pull request Jan 13, 2016
@bors-servo
Auto merge of #9107 - ecoal95:webgl-uniforms-and-nits, r=simartin
Loading status checks…
c13e840 
webgl: Track the current program, implement some uniform functions, and nits

Was done while implementing sequence arguments.

Depends on #9056.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 4092ffd into servo:master Jan 13, 2016
2 checks passed
2 checks passed
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
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@emilio @bors-servo @jdm @simartin @KiChjang @highfive
You can’t perform that action at this time.