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

Remove DepthWrappingVertex<T> #5943

Merged
merged 6 commits into from
Aug 15, 2023
Merged

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jul 25, 2023

This is a breaking change so I suggest applying it as late as possible as there will be other breaking changes.

Taking the opportunity to make a bit of a breaking change while cleaning this up. The existence of this object is a bit weird - it's as if it's an implementation detail yet is not treated as an implementation detail - you need to be very well aware that this is a thing when implementing renderers.


vNext

TexturedVertex2D must be initialised with an IRenderer.

The default constructor for this type has been obsoleted and will now generate a compiler error.

@smoogipoo smoogipoo mentioned this pull request Jul 28, 2023
4 tasks
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Code looks quaint enough. Will run some platform testing later but I don't expect surprises.


public TexturedVertex2D(IRenderer renderer)
{
this = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL that this is legal C#.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#12813-this-access:

When this is used in a primary_expression within an instance constructor of a struct, it is classified as a variable. The type of the variable is the instance type (§15.3.2) of the struct within which the usage occurs, and the variable represents the struct being constructed.

Copy link
Member

Choose a reason for hiding this comment

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

Might be legal, but I'd have no idea what this means nor how to quickly find the documentation for what it means 😅 . Might be worth a comment?

Copy link
Collaborator

@bdach bdach Aug 14, 2023

Choose a reason for hiding this comment

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

It's just a shorthand to initialise all members of the struct to default values for their type, as explicit initialisation is required for structs. Easier to write this = default; than to do this per member in both ctors.

Can agree with inline comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Inline comment added in 3da4187

@bdach bdach enabled auto-merge August 15, 2023 07:11
@bdach bdach merged commit 6799b1f into ppy:master Aug 15, 2023
10 of 12 checks passed
@smoogipoo smoogipoo deleted the remove-depthwrappingvertex branch September 11, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants