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

Fix typos #13571

Closed
wants to merge 3 commits into from
Closed

Fix typos #13571

wants to merge 3 commits into from

Conversation

Lioness100
Copy link

This PR fixes a few typos throughout the repository. I noticed a few typos that were either part of public APIs, or that I didn't know what they should be corrected to:

  • SimplicationQueueSceneComponent: should be SimplificationQueueSceneComponent, but it looks public
  • recordChunckSize: should be recordChunkSize, but it looks public
  • volumetricLightMergePostProces: should be volumetricLightMergePostProcess, but it looks public
  • supressXRSelectEvents: should be suppressXRSelectEvents, but it looks public
  • disableSpecatatorMode: should be disableSpectatorMode, but it looks public
  • dipose: should be dispose, but it looks public
  • diffuseTexturY: should be diffuseTextureY. This is from the following code:
    @serializeAsTexture("diffuseTexturY")
    private _diffuseTextureY: BaseTexture;

The typo is only in the decorator, so I don't know if changing it would be breaking.

  • locatRect: located here. I don't know if this spelling is correct, or if it should be localRect, or maybe locateRect?
  • surexposed: located here, not sure if it's a typo
  • rewrited: located here. I don't really understand the intent of the sentence due to some grammar issues, so I'm not sure the best way to fix it.
  • Wwves: located here. Should this be "Waves"?
  • onFullcreenRequiredObservable: should be onFullscreenRequiredObservable, but it looks public
  • onEditorFullcreenRequiredObservable: onEditorFullscreenRequiredObservable, but it looks public
  • shadown: located in a few places, is this a typo? Maybe for shadow or shadows?
  • shadowSpotlLightConfiguration: I think it should be shadowSpotlightConfiguration, but it looks public
  • SphericalPolynomalCoefficients: should be SphericalPolynomialCoefficients, but it looks public
  • rotatquatRotationY: is this a typo?

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

There is no need to update the "what's new.md" file. A changelog will be generated using the PR and its tags.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

There is no need to update the "what's new.md" file. A changelog will be generated using the PR and its tags.

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

Wow, what an awesome PR. Love it! :-)

I found a few public API changes that should be proxied - a deprecated getter would be the best. Otherwise - this is amazing.

Comment on lines +95 to +100
public get minorLineThickness(): number {
return this._minorLineThickness;
}

public set minorLineTickness(value: number) {
this._minorLineTickness = value;
public set minorLineThickness(value: number) {
this._minorLineThickness = value;
Copy link
Member

Choose a reason for hiding this comment

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

Wheereas I totally agree that this is a typo and should be fixed, this is a breaking change. We will need to add a (deprecated) proxy to support the old syntax

Comment on lines +119 to +124
public get majorLineThickness(): number {
return this._majorLineThickness;
}

public set majorLineTickness(value: number) {
this._majorLineTickness = value;
public set majorLineThickness(value: number) {
this._majorLineThickness = value;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@@ -460,7 +460,7 @@ export class MRDLSliderBarMaterial extends PushMaterial {
/**
* @internal
*/
public globaRightIndexTipPosition = new Vector4(0.0, 0.0, 0.0, 1.0);
public globalRightIndexTipPosition = new Vector4(0.0, 0.0, 0.0, 1.0);
Copy link
Member

Choose a reason for hiding this comment

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

back-compat

Comment on lines +110 to +114
public get highlighterOpacity(): number {
return this._highlighterOpacity;
}

public set highligherOpacity(value: number) {
if (this._highligherOpacity === value) {
public set highlighterOpacity(value: number) {
Copy link
Member

Choose a reason for hiding this comment

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

back-compat

@bjsplat
Copy link
Collaborator

bjsplat commented Feb 28, 2023

@RaananW
Copy link
Member

RaananW commented Feb 28, 2023

This PR fixes a few typos throughout the repository. I noticed a few typos that were either part of public APIs, or that I didn't know what they should be corrected to:

  • SimplicationQueueSceneComponent: should be SimplificationQueueSceneComponent, but it looks public

Public, I would rather not change it. But you are right :-)

  • recordChunckSize: should be recordChunkSize, but it looks public

Same. As for all public APIs - either we define a deprecated getter for them (in case of members or getters), or sadly leave them as they are

  • dipose: should be dispose, but it looks public

we have a dipose function somewhere? this would feel like a bug.

  • diffuseTexturY: should be diffuseTextureY. This is from the following code:
    @serializeAsTexture("diffuseTexturY")
    private _diffuseTextureY: BaseTexture;

The typo is only in the decorator, so I don't know if changing it would be breaking.

I would consider that a bug - we should change that.

  • locatRect: located here. I don't know if this spelling is correct, or if it should be localRect, or maybe locateRect?

should be local

  • surexposed: located here, not sure if it's a typo
  • rewrited: located here. I don't really understand the intent of the sentence due to some grammar issues, so I'm not sure the best way to fix it.

is rewritten, instead of be rewrited. But I think the 2nd half of the sentence is not really needed TBH. Keep it as is for now.

  • Wwves: located here. Should this be "Waves"?

Waves for sure :-)

  • shadown: located in a few places, is this a typo? Maybe for shadow or shadows?

Shadows, probably. depending on the context.

  • rotatquatRotationY: is this a typo?

is this public? yes - this must be a typo

@Lioness100
Copy link
Author

Lioness100 commented Feb 28, 2023

we have a dipose function somewhere? this would feel like a bug.

Shadows, probably. depending on the context.

shadownEnabled?: boolean; // only on specific lights!

const shadownMap = shadowGenerator.getShadowMap();

is this public? yes - this must be a typo

No, it's a local variable name.

const rotatquatRotationionY = Quaternion.RotationAxis(Axis.Y, rotationY || 0);

const rotatquatRotationionY = Quaternion.RotationAxis(Axis.Y, environmentMapConfiguration.rotationY || 0);

@RaananW
Copy link
Member

RaananW commented Feb 28, 2023

we have a dipose function somewhere? this would feel like a bug.

Shadows, probably. depending on the context.

shadownEnabled?: boolean; // only on specific lights!

const shadownMap = shadowGenerator.getShadowMap();

is this public? yes - this must be a typo

No, it's a local variable name.

const rotatquatRotationionY = Quaternion.RotationAxis(Axis.Y, rotationY || 0);

const rotatquatRotationionY = Quaternion.RotationAxis(Axis.Y, environmentMapConfiguration.rotationY || 0);

oh, all of these can be changed :-)
dispose (a bug), shadowsEnabled (a bug), shadowsMap, quaternionRotationY

@RaananW
Copy link
Member

RaananW commented Feb 28, 2023

And once again, thank you so much for all the time invested in this!

@Lioness100
Copy link
Author

No problem! A bit busy atm, but I'll try to make the new changes within the next few days.

@RaananW
Copy link
Member

RaananW commented Mar 14, 2023

Moving this to draft for the time being, will move it back when you have the time :-)

@RaananW RaananW marked this pull request as draft March 14, 2023 12:26
@sebavan
Copy link
Member

sebavan commented Jun 1, 2023

@Lioness100 do you have any updates on this PR ?

@sebavan
Copy link
Member

sebavan commented Jun 14, 2023

Closing for now and let s reopen if it gets updated.

@sebavan sebavan closed this Jun 14, 2023
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.

4 participants