Skip to content

Conversation

WestLangley
Copy link
Collaborator

As suggested here and here -- so it must be a good idea. ;-)

@WestLangley WestLangley added this to the r179 milestone Jul 26, 2025
Copy link

github-actions bot commented Jul 26, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 338.39
78.91
338.39
78.91
+1 B
+0 B
WebGPU 566.01
156.49
566.01
156.49
+0 B
+0 B
WebGPU Nodes 564.61
156.25
564.61
156.25
+0 B
+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 469.77
113.61
469.82
113.62
+49 B
+14 B
WebGPU 637.21
172.44
637.26
172.46
+48 B
+14 B
WebGPU Nodes 591.86
161.68
591.9
161.69
+48 B
+15 B

@WestLangley WestLangley changed the title Src: Make camera.reversedDepth private Src: Make camera.reversedDepth private Jul 26, 2025
Fix JSDoc.
@sunag
Copy link
Collaborator

sunag commented Jul 27, 2025

I would only vote against this change if we planned to use this property in external components with addons in the future.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 27, 2025

I think we can still do that. But if reversedDepth is a public and documented camera property, users might assign values to it which is probably not something we want.

@sunag
Copy link
Collaborator

sunag commented Jul 27, 2025

camera.reversedDepth seems to follow the same logic as camera.coordinateSystem where only the renderer sets these properties. The current logic should apply to both.

Maybe because I come from C# too, but accessing private variables in external components doesn't seem to make much sense to me, could this just be documented?

@sunag
Copy link
Collaborator

sunag commented Jul 27, 2025

But if reversedDepth is a public and documented camera property, users might assign values to it which is probably not something we want.

Maybe we can document it as a "volatile" value that is defined by the renderer. Alternatively, use get reversedDepth and set it through _reversedDepth in renderer.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 27, 2025

Alternatively, use get reversedDepth and set it through _reversedDepth in renderer.

That sounds good to me.

@WestLangley WestLangley force-pushed the dev-reversed_depth_private branch from 30a38c6 to 7e30989 Compare July 28, 2025 00:54
@Mugen87 Mugen87 merged commit 1fffae9 into mrdoob:dev Jul 28, 2025
9 checks passed
@Mugen87 Mugen87 changed the title Src: Make camera.reversedDepth private Camera: Make reversedDepth read-only. Jul 28, 2025
@WestLangley WestLangley deleted the dev-reversed_depth_private branch July 28, 2025 19:31
Comment on lines +70 to 74
get reversedDepth() {

return this._reversedDepth;

}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see #31512 (comment).

This should be read-only for users, and set only by the renderer.

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