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 Projection::invert for orthographic projection #95303

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

EIREXE
Copy link
Contributor

@EIREXE EIREXE commented Aug 8, 2024

Fixes #68878, specially when using orthographic projection.

Replaces our invert implementation with mesa's, which seems to work fine.

@EIREXE EIREXE requested a review from a team as a code owner August 8, 2024 22:01
@Calinou Calinou added bug topic:core cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 8, 2024
@Calinou Calinou added this to the 4.4 milestone Aug 8, 2024
@clayjohn
Copy link
Member

clayjohn commented Aug 9, 2024

Do we have tests for this? If not, this PR should add some

@akien-mga akien-mga changed the title Fix Projection::invert Fix Projection::invert for orthographic projection Aug 9, 2024
@EIREXE EIREXE force-pushed the inverted_composer branch from 65c8ccc to 2f08b7d Compare August 9, 2024 18:11
@EIREXE EIREXE requested a review from a team as a code owner August 9, 2024 18:11
@EIREXE
Copy link
Contributor Author

EIREXE commented Aug 9, 2024

Added a few tests too

core/math/projection.cpp Outdated Show resolved Hide resolved
core/math/projection.cpp Outdated Show resolved Hide resolved
core/math/projection.cpp Outdated Show resolved Hide resolved
core/math/projection.cpp Outdated Show resolved Hide resolved
tests/core/math/test_projection.h Outdated Show resolved Hide resolved
tests/test_main.cpp Show resolved Hide resolved
@EIREXE EIREXE force-pushed the inverted_composer branch from 2f08b7d to 3aa46b9 Compare August 9, 2024 20:21
@EIREXE
Copy link
Contributor Author

EIREXE commented Aug 9, 2024

Made all expressions into their own lines and added back the accidentally removed test.

@AThousandShips AThousandShips removed their request for review October 1, 2024 09:16
@EIREXE EIREXE force-pushed the inverted_composer branch from 3aa46b9 to 3efcb0c Compare October 2, 2024 18:47
core/math/projection.cpp Outdated Show resolved Hide resolved
@clayjohn
Copy link
Member

clayjohn commented Oct 3, 2024

Testing locally and I can confirm that this fixes the issue when using an orthographic projection. I also tested performance. With a debug build this version is quite a bit faster than the old version. With a release build there is no difference in performance.

So we just need to decide what to do about the licensing, and then this should be fine to merge for 4.4

Spartan322 pushed a commit to Spartan322/redot-engine that referenced this pull request Oct 22, 2024
Relates to godotengine/godot#68878, specially when using orthographic projection.

Also adds some tests.

Adapted from godotengine/godot#95303

Co-authored-by: Álex Román <eirexe@eirteam.moe>
Spartan322 pushed a commit to Spartan322/redot-engine that referenced this pull request Oct 22, 2024
Relates to godotengine/godot#68878, specially when using orthographic projection.

Also adds some tests.

Adapted from godotengine/godot#95303
Spartan322 pushed a commit to Spartan322/redot-engine that referenced this pull request Oct 22, 2024
Relates to godotengine/godot#68878, specially when using orthographic projection.

Also adds some tests.

Adapted from godotengine/godot#95303
Spartan322 pushed a commit to Spartan322/redot-engine that referenced this pull request Oct 22, 2024
Relates to godotengine/godot#68878, specially when using orthographic projection.

Also adds some tests.

Adapted from godotengine/godot#95303

(cherry picked from commit 0773028)
Fixes godotengine#68878, specially when using orthographic projection.

Also adds some tests.
@akien-mga akien-mga merged commit 23fc8e2 into godotengine:master Nov 20, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INV_PROJECTION_MATRIX incorrect for orthogonal cameras
6 participants