Skip to content

Conversation

@Richard-Yunchao
Copy link

PTAL.

See the investigation about Viewport and Scissor Test at #120

@Richard-Yunchao
Copy link
Author

@Kangz , @kainino0x , @kvark , @RafaelCintron , @litherum , PTAL when you have time.

Copy link
Contributor

@Kangz Kangz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm not sure we should include "spec language" in the IDL though.

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should bring this up on the call (documenting in IDL).

// The default viewport is (0.0, 0.0, w, h, 0.0, 1.0), where w and h are the dimensions of back buffer
void setViewport(float x, float y, float, width, float height, float minDepth, float maxDepth);

// The default scissor rectangle is (0, 0, w, h), where w and h are the demensions of back buffer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "demensions"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for your review, @kvark

@kvark
Copy link
Contributor

kvark commented Nov 30, 2018

@Richard-Yunchao please squash the commits

@Richard-Yunchao
Copy link
Author

@Richard-Yunchao please squash the commits

Done. I remember that we can merge multiple commits into one commit when we merge the pr. So I didn't squash the commits before, @kvark .

If you prefer to squash commits into one commit, I will do that in future.

@kvark
Copy link
Contributor

kvark commented Dec 3, 2018

@Richard-Yunchao you are right. I just got used to bad automerge bots that don't know how to squash :)

@Richard-Yunchao
Copy link
Author

Can you take a look? @kvark . Shall we remove the comments for these two functions in this pr?

@litherum litherum self-requested a review December 3, 2018 19:41
@litherum
Copy link
Contributor

litherum commented Dec 3, 2018

In the future we may want support for multiple viewports (e.g. for VR). We should keep that future direction in mind.

void setStencilReference(u32 reference);

// The default viewport is (0.0, 0.0, w, h, 0.0, 1.0), where w and h are the dimensions of back buffer
void setViewport(float x, float y, float, width, float height, float minDepth, float maxDepth);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rouge comma (,) between float width. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in prep for merge. Thanks for finding this!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Kangz
Copy link
Contributor

Kangz commented Dec 3, 2018

Approved in the 2018-12-03 meeting with the typo fix @frzi mentioned.

@Kangz
Copy link
Contributor

Kangz commented Dec 3, 2018

Aaaand @kainino0x fixed it, nice!

@Kangz Kangz merged commit d9a31d4 into gpuweb:master Dec 3, 2018
@Richard-Yunchao
Copy link
Author

In the future we may want support for multiple viewports (e.g. for VR). We should keep that future direction in mind.

Yes. I also simply discussed with a few guys about multiple viewports for WebVR several days ago. In addition, I added this statement into section at the investigation issue for Viewport and scissor tests. Thanks for your reminder, @litherum.

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.

6 participants