-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add Custom Projection to Camera3D #84454
base: master
Are you sure you want to change the base?
Conversation
Can you test what happens if you create a projection that produces invalid output values, like |
Actually this implementation affect just the projection matrix it works very well with oblique projection... , then it's for people that want to do some tweaks like 3d looks parallel in one dimensions, then it doesn't cause issues with viewports ..., I think it's so useful. |
What do you mean? I'm not asking for that, I'm asking what happens if you provide a broken |
I tested playing with the projection as i can it never crash the rendering. |
Try with something like: If it causes problems then that would be helped by having the methods suggested in: |
I tested it, then it does not cause any problems, even with the frustum. |
I also need this for my project, and I also have my own implementation (roughly the same): https://github.com/PitouGames/godot/tree/custom_projection I don't understand why the projection matrix is accessible in the editor GUI in this PR after your comment: godotengine/godot-proposals#2713 (comment). User will still be able to show it with scripts like that:
There are the camera's functions Can we also take advantage of this PR to move the field "Current" up? It's in the middle of projection settings. |
While we're at it, I'd move Current at the very top and move Keep Aspect just above FOV. The way FOV is interpreted (vertical or horizontal) depends on the value of Keep Aspect. |
Actually I just make it accessible because it makes it a little bit harder to test some values theoretically then making it accessible from the editor is pointless as the projection must be recalculated if the viewport had resized but still annoying to export another projection as a script variable then make a setter..., I think it's OK now. |
@BrunkMortel I'll check it because I don't have this issue in my projects, can provide which renderer are you using? |
@DxUr I'm using the Forward+ renderer and everything else is by default. |
Tested locally (rebased against
This only occurs if SDFGI is enabled. VoxelGI doesn't have these errors but renders incorrectly if any values in the custom projection are modified. Also, running a project with a Camera3D that has its projection mode to Custom prints the following error (even if run outside the editor):
Directional shadow rendering also breaks if you change any values in the custom camera projection, with the following errors printed:
Point light shadow rendering breaks with certain values too (even though the rest of the scene still renders), but not as often as directional shadows. There are also no errors printed in this case. Testing project: test_pr_84454.zip |
@DxUr what's the status on this? Is there work to be done or are you waiting on more feedback? |
@DxUr I would like to continue working on this PR, since it's nearly complete and I would benefit from it. |
I will fix all the issues that people face with this PR as soon as possible so I would like to be merged before 4.3 |
Co-authored-by: A Thousand Ships <[email protected]>
… own function for standardization
…a_3d with custom projection
a69bea8
to
c637a49
Compare
Thank you for fixing all the issues, I checked your solution and I think it's enough. |
c637a49
to
e360ab6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if any of these are blockers, as the PR works as-is.
Vector<Plane> planes = c_proj.get_projection_planes(Transform3D()); | ||
const Projection::Planes intersections[8][3] = { | ||
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | ||
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | ||
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | ||
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | ||
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | ||
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | ||
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | ||
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | ||
}; | ||
|
||
for (int i = 0; i < 8; i++) { | ||
Plane a = planes[intersections[i][0]]; | ||
Plane b = planes[intersections[i][1]]; | ||
Plane c = planes[intersections[i][2]]; | ||
ERR_FAIL_COND_MSG(!a.intersect_3(b, c), "Camera3D custom projection has non-intersecting planes; skipping update."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I think this check should be in set_custom_projection
instead, before c_proj
is ever set. I'm also not sure if it makes sense to disallow these projections, since it seems like only lighting and the editor gizmo are dependent on having endpoints, if someone familiar could weigh in.
Edit: Okay actually I'm not too sure if changing this makes sense, since moving it before c_proj
is set means you can't change the value through the invalid values in the editor.
Vector<Plane> planes = c_proj.get_projection_planes(Transform3D()); | |
const Projection::Planes intersections[8][3] = { | |
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | |
}; | |
for (int i = 0; i < 8; i++) { | |
Plane a = planes[intersections[i][0]]; | |
Plane b = planes[intersections[i][1]]; | |
Plane c = planes[intersections[i][2]]; | |
ERR_FAIL_COND_MSG(!a.intersect_3(b, c), "Camera3D custom projection has non-intersecting planes; skipping update."); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, it seems that for testing only the planes for Transform3D()
is not sufficient, since it seems that it is still possible to have no endpoints if the camera position/rotation is different, which means the reported vulkan crash can still happen, albeit rarely (I spent about 30 seconds dragging every projection value back and forth before getting a crash). I don't think testing every time the camera transform changes makes sense, so there should be a more robust way of checking if a projection has endpoints regardless of transform?
FWIW you can replicate the error without a custom projection by setting a funky transform (e.g. scale = 0), which isn't guarded against, so I also have to wonder if it makes sense to guard against weird projections in the first place.
void Camera3D::set_projection(ProjectionType p_mode) { | ||
if (p_mode == PROJECTION_PERSPECTIVE || p_mode == PROJECTION_ORTHOGONAL || p_mode == PROJECTION_FRUSTUM) { | ||
if (p_mode == PROJECTION_CUSTOM && mode != PROJECTION_CUSTOM && is_inside_tree()) { | ||
c_proj = get_camera_projection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't always work as expected in the editor1. However, it works fine at runtime. I do wonder if it is better not to set the custom projection this way though, and instead let it keep its existing value, since it is trivial to retain the existing projection anyway, and most of the time when setting a custom projection it's something not possible with the other projection modes.
Footnotes
-
get_viewport
seems to give the 2D viewport, which has its size out of sync with the 3D viewport sometimes (e.g. when the editor is first opened, or when the viewport area is resized, the viewport which isn't being rendered doesn't have its size updated) ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these need to be updated somehow, but am not really sure how.
@@ -641,6 +702,11 @@ void Camera3D::set_far(real_t p_far) { | |||
_update_camera_mode(); | |||
} | |||
|
|||
void Camera3D::set_custom_projection(Projection p_proj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to the other comment.
void Camera3D::set_custom_projection(Projection p_proj) { | |
void Camera3D::set_custom_projection(Projection p_proj) { | |
Vector<Plane> planes = p_proj.get_projection_planes(Transform3D()); | |
const Projection::Planes intersections[8][3] = { | |
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_FAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_FAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_LEFT, Projection::PLANE_BOTTOM }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_TOP }, | |
{ Projection::PLANE_NEAR, Projection::PLANE_RIGHT, Projection::PLANE_BOTTOM }, | |
}; | |
for (int i = 0; i < 8; i++) { | |
Plane a = planes[intersections[i][0]]; | |
Plane b = planes[intersections[i][1]]; | |
Plane c = planes[intersections[i][2]]; | |
ERR_FAIL_COND_MSG(!a.intersect_3(b, c), "Camera3D custom projection has non-intersecting planes; skipping update."); | |
} |
@otonashixav But it works fine for me, I had tried every possible case! |
I mean it does work fine. I'm just concerned about some points that the reviewers might be able to clear up. |
9ec1408
to
eb4bacf
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This is not a bug, this is expected due to what a projection matrix is. The Projection matrix is 4x4 Matrix, you should not change it manually you must use a script to do that, Than you can fix the tilting by playing with all components. |
In this case, it should be marked read-only in the inspector to prevent accidental changes. This can be done using |
Is the |
Thanks for writing this PR. I don't mind building from source but would love to see this merged so I can be confident relying on it in my project. Are there any remaining blockers? If more needs to be done, I'd be happy to help out. |
This hasn't been reviewed entirely yet, so it needs to be reviewed properly first, so nothing you can help with necessarily, we're getting close to feature freeze as well meaning this is likely not going to be in 4.3 |
FYI I attended the Rendering Meeting, the main takeaway was that without screen space effects and all forward lighting pathways working with custom projection this would not get merged: If you are using a custom projection with capacity to fully change the matrix I am not sure if you can deliver all those features reliably or not. Verifying what you can support lighting and effects wise and under what conditions seems to be the key to advancing this PR. |
Add custom_projection property to Camera3D