Skip to content

Conversation

@WestLangley
Copy link
Collaborator

I thought it would be nice if the method could be deemed correct simply by inspection. :-)

The implementation is now similar to that of makePerspective().

@WestLangley WestLangley added this to the r179 milestone Jul 20, 2025
@github-actions
Copy link

github-actions bot commented Jul 20, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 338.04
78.86
338.04
78.86
+0 B
+0 B
WebGPU 559.1
154.77
559.1
154.77
+0 B
+0 B
WebGPU Nodes 558.02
154.56
558.02
154.56
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 469.29
113.56
469.31
113.52
+16 B
-42 B
WebGPU 634.39
171.81
634.4
171.77
+16 B
-44 B
WebGPU Nodes 589.52
161.12
589.54
161.08
+16 B
-43 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 20, 2025

It seems the following unit test fails now:

not ok 1854 Maths > Frustum > setFromProjectionMatrix/makeOrthographic/containsPoint

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 20, 2025

@OrangeDoro Please stop writing these comments. They are considered as SPAM.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 21, 2025

So I guess after #31456 a rebase should fix the CI?

@bhouston
Copy link
Contributor

bhouston commented Jul 21, 2025

You've increased divides, which are the slowest operation, by 2x. From 3 to 6 no? FP divides are 2x to 5x slower than multiplications, but this is a micro-optimization.

The previous pattern of precalculating the inverses was the most efficient and pretty standard across math libraries.

@WestLangley
Copy link
Collaborator Author

This method is not called in a tight loop. Nor is it computationally expensive.

Compare makeOrthographic()in this PR with makePerspective(), and see how similar they are now.

Our policy has been to write for clarity first, and modify for speed if there is a demonstrated performance issue.

If this PR is merged, it would be fine with me to add the suggested micro-optimizations to both makeOrthographic()and makePerspective() in a subsequent PR, but I don't think doing so is necessary in this case.

@Mugen87 Mugen87 merged commit a681a5f into mrdoob:dev Jul 21, 2025
9 checks passed
@WestLangley WestLangley deleted the dev-make_orthographic branch July 21, 2025 21:09
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.

3 participants