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

Allow for creating a zero size plane #16052

Merged
merged 2 commits into from
Jan 14, 2025
Merged

Conversation

amirt-ms
Copy link
Contributor

@amirt-ms amirt-ms commented Jan 9, 2025

I have a use case for creating a plane with zero width/height and then updating it live. The current method of checking whether a size/width/height is passed does not allow this. These changes check if the value is defined instead of is truthy.

I have a use case for creating a plane with zero width/height and then updating it live. The current method of checking whether a size/width/height is passed does not allow this. These changes check if the value is defined instead of is truthy.
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 9, 2025

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 Jan 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 9, 2025

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

This is a breaking change and I am not sure to understand why it is usefull if you update the values ?

@amirt-ms
Copy link
Contributor Author

amirt-ms commented Jan 9, 2025

This is a breaking change and I am not sure to understand why it is usefull if you update the values ?

Can you please explain what this breaks?

I want to construct a plane of zero size. I cannot do this in the current implementation because 0 is falsy, so the code defaults to a size of 1, even though I passed size as an argument. In my opinion, this is a bug.

@deltakosh
Copy link
Contributor

It is a breaking change because users (like in NGE) can assume 0 as "default":

options.size = this.size.getConnectedValue(state);

@deltakosh
Copy link
Contributor

We have two ways here:

  • We can have a special case where if size === -1, then we construct it with size = 0. Fishy but compatible
  • We break compat, update NGE and only use undefined for real undefined values

@deltakosh
Copy link
Contributor

Actually option 2 is fine. THe CreatePlaneVertexData is not used by nge

You can go ahead with just that slight change: there is no need to use typeof, simply check option.width !== undefined

@amirt-ms
Copy link
Contributor Author

Actually option 2 is fine. THe CreatePlaneVertexData is not used by nge

You can go ahead with just that slight change: there is no need to use typeof, simply check option.width !== undefined

Thank you for this information; I made this change here: 45d9bbc. Please let me know if any further changes should be made.

@deltakosh deltakosh enabled auto-merge (squash) January 14, 2025 18:21
@amirt-ms amirt-ms requested a review from sebavan January 14, 2025 18:41
@deltakosh deltakosh merged commit f59864a into BabylonJS:master Jan 14, 2025
14 checks passed
@amirt-ms amirt-ms deleted the patch-1 branch January 14, 2025 22:45
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.

4 participants