Include the follow-viewport-transform into CanvasLayer transform calculations #59682
Conversation
Sauermann
changed the title
There was a problem hiding this comment.
Added in docs descriptions are all inverted. Also probably other transform-getting methods omitted in this PR should be precised in a similar way as well (e.g. CanvasItem::get_global_transform: "Transforms from local coordinate system of this [CanvasItem] to the to the coordinate system of the canvas, this item is in.
Regarding the code changes they look good (besides the comments). Can't tell if there are any more places where CanvasLayer::get_final_transform() would need to be used now.
| follow.scale(Vector2(get_follow_viewport_scale(), get_follow_viewport_scale())); | ||
| if (vp) { | ||
| follow = vp->get_canvas_transform() * follow; | ||
| } |
There was a problem hiding this comment.
Should follow scale be applied if vp is invalid? 🤔
There was a problem hiding this comment.
This is a philosophical question. I would be happy to adjust the PR, acording to the preference of the maintainers.
Sauermann
force-pushed
the
fix-following-viewport-transform
branch
from
fc02c8b
to
3fe7b50
Compare
That sounds like a good idea, however that should be done in a separate PR, since this is solely about the referenced issue.
When I created this PR half a year ago, I investigated all places, where CanvasLayer::get_transform was used, so I am quite certain, that i found all relevant places. I can not be sure, if there were other changes made in the meantime that make would make additional changes necessary. |
Now I have double checked all locations, where |
Sauermann
force-pushed
the
fix-following-viewport-transform
branch
from
3fe7b50
to
6bc0f0f
Compare
There was a problem hiding this comment.
LGTM.
Still not sure about that follow scale part which I've commented on above but also not sure if it's anyhow relevant.
Regarding the docs consistency for different transform obtaining methods: these descriptions can be improved/unified properly later, after other related bug fixes and after/alongside fixing the approprite tutorial (see: godotengine/godot-docs#6277). So docs in here are fine as they're correct.
And most importantly: big thanks @Sauermann! Great work on this and other related PRs/issues!
|
Is this fix in v3.5.2.rc1.official [f5f0543]? I'm still having a problem with a CollisionObject2D inside a CanvasLayer in reference to the camera position. |
|
No, at this time it is neither in 3.x nor in 4. |
|
Is there a workaround to this? Clicking on things in a moving world is a major mechanic of my game. I tried moving the nodes inside the CanvasLayer with combinations of camera.get_global_position() and display/window/size/ with partial success, but it seems to cut off parts of the screen the mouse can be detected on. |
|
A workaround, I can think of is to replace the |
The following-viewport-transform was missing from several calculations
Sauermann
force-pushed
the
fix-following-viewport-transform
branch
from
6bc0f0f
to
2da2220
Compare
|
Thanks! |
|
@akien-mga #35965 which this PR fixes still needs to be fixed in 3.x, I guess it's fine to cherry-pick this PR as it only adds a new method? |
akien-mga
added
the
cherrypick:3.x
|
Yeah I suppose so, if it works :) |
|
It would be prudent to check, if |
akien-mga
removed
the
cherrypick:3.x
…ulations The follow-viewport-transform was missing from several calculations 3.x version of #59682
…ulations The follow-viewport-transform was missing from several calculations 3.x version of godotengine#59682
…ulations The follow-viewport-transform was missing from several calculations 3.x version of godotengine#59682
…ulations The follow-viewport-transform was missing from several calculations 3.x version of godotengine#59682
…ulations The follow-viewport-transform was missing from several calculations 3.x version of godotengine#59682 (cherry picked from commit 608cbd8)
The follow-viewport-transform was missing from several calculations
resolve #35965
Update 2022-10-07: fix merge conflict