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

Refactor Frustum to use Plane #6784

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Refactor Frustum to use Plane #6784

merged 8 commits into from
Jul 4, 2024

Conversation

willeastcott
Copy link
Contributor

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@willeastcott willeastcott merged commit 351a919 into main Jul 4, 2024
8 checks passed
@willeastcott willeastcott deleted the frustum-planes branch July 4, 2024 16:53
@@ -11,7 +11,6 @@ class Plane {
* The normal of the plane.
*
* @type {Vec3}
* @readonly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of removing it in Plane class I would rather add @readonly to Frustum#planes aswell. It doesn't mean that you shouldn't update the content, but only that you guarantee the instance of the array will be the same all the time. E.g. instead of "GC causing cleaning" like many people would do via frustum.planes = [] it be invalid and instead frustum.planes.length = 0 should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is perfectly valid to do whatever you like to the plane's normal Vec3. Replace it, edit it, whatever - nothing will break. So it's right to remove it. As for the planes property, again, if you really want to replace the array with something else, is that really a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example #2088, issue is the ease of introducing problems in which, for instance, Vec3.ZERO has been overwritten. So some developers basically tried to settle on prefering ownership of variables in classes and not mixing around internal variables with other internal variables in the hope it creates code that is easier to quickly argue about, easier to maintain and allowing more idiomatic code (e.g. ability to use Vec3 constants in constructors).

If that is not the case in your observation, you could of course also remove all the other @readonly's aswell that were added just some time ago. But idk, clearly different people pull in different directions which creates some friction, so it might be time to finally reach a consensus on this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed we need to keep the @readonly here, to avoid possible issues we've just removed in other shapes.

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