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

GraphicsDevice properties refactor #4013

Merged
merged 3 commits into from
Feb 11, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Feb 10, 2022

This PR moves some properties which need to be public, as well as some shared code, from WebglGraphicsDevice back to GraphicsDevice. This is a follow up for #4009 and #4010.

API Changes - which are expected to have no impact to existing projects:

Following properties and functions are no longer exposed as public in GraphicsDevice class, as they have no use case:

updateBegin	
updateEnd	
clear	
copyRenderTarget
draw	

getBlending	
getDepthTest	
getDepthWrite	
getRenderTarget	
setBlendEquation	
setBlendEquationSeparate	
setBlendFunction	
setBlendFunctionSeparate	
setBlending	
setColorWrite	
setCullMode	
setDepthFunc	
setDepthTest	
setDepthWrite	
setIndexBuffer	
setRenderTarget	
setScissor	
setShader	
setStencilFunc	
setStencilFuncBack	
setStencilFuncFront	
setStencilOperation	
setStencilOperationBack	
setStencilOperationFront	
setStencilTest	
setVertexBuffer	
setViewport	

Additionally, VertexFormat.update has been deprecated, as well as VertexFormat.elements changed into readonly, as the engine does not support those being modified.

@mvaligursky mvaligursky self-assigned this Feb 10, 2022
@mvaligursky mvaligursky requested a review from a team February 10, 2022 15:18
*
* @type {HTMLCanvasElement}
*/
canvas;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering how this code gets executed (and also converted to es5).

This line is declaring a member and could also be initialising it, like canvas = null;. But the constructor does this.canvas = canvas;. So does the code on this line run after the constructor's call to super();, but before the rest of the constructor code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, when you step over this in the debugger, those things execute first (and will be set to undefined), before constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what it looks like in ES5
Screenshot 2022-02-11 at 09 48 45

Copy link
Member

Choose a reason for hiding this comment

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

Interesting.

So could you do canvas = canvas; here, instead of doing it in the constructor?

In a class as large as this it probably makes little difference, but in general we don't want to be initialising members twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, you don't have constructor parameters available at that point in ES6 world. Only in ES5 it's transpiled to doing it all inside constructor.

Copy link
Member

Choose a reason for hiding this comment

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

So I guess in general we want to declare (and optionally initialise) variables here or in the constructor, but not both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt that would make any perf difference .. but perhaps it'd be useful to test it in a class such as Vec3?

Copy link
Contributor

Choose a reason for hiding this comment

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

So yep, I probably wouldn't add that to Vec3. I considered it but so many are created, I decided to avoid potentially slowing the code down. But for classes that are rarely created, I think it's stylistically very nice (and actually maps directly to TypeScript).

Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the idea instance members are initialised twice in our constructors, it's gross.

However reading here, they say private versions of these (which we hope to use in future) must be declared this way:

Note: Private fields can only be declared up-front in a field declaration.

So it seems we ultimately won't have a choice and hopefully compiler will optimise away the redundant set.

@mvaligursky mvaligursky merged commit 5932dec into dev Feb 11, 2022
@mvaligursky mvaligursky deleted the mvaligursky-device-refactor-properties branch February 11, 2022 10:44
@mvaligursky mvaligursky mentioned this pull request Jul 11, 2022
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