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

Add position rect to camera #2101

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

alejcas
Copy link
Member

@alejcas alejcas commented Jun 4, 2024

Camera has rects for viewport and projection but not for position.
I use a lot the camera rect to obtain the map space rect currently on screen.

Copy link
Collaborator

@DragonMoffon DragonMoffon left a comment

Choose a reason for hiding this comment

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

Because the Camera2D can rotate, it doesn't make sense to return a rect as you have defined it. This is a useful feature, but there is a few changes I would recomend.

arcade/camera/camera_2d.py Outdated Show resolved Hide resolved
@DigiDuncan
Copy link
Collaborator

Like this idea a lot. Would love to see it get in, and as the person spearheading Rect, I definitely love hearing that it's getting practical use!

@alejcas
Copy link
Member Author

alejcas commented Jun 4, 2024

Like this idea a lot. Would love to see it get in, and as the person spearheading Rect, I definitely love hearing that it's getting practical use!

Using Rects as if there was no tomorrow!

Copy link
Collaborator

@DragonMoffon DragonMoffon left a comment

Choose a reason for hiding this comment

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

nice!

Copy link
Member

@Cleptomania Cleptomania left a comment

Choose a reason for hiding this comment

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

I'm just sanity checking that the behavior makes sense.

If the camera can rotate, should the returned Rect be accounting for the rotation(as in, returning a Rect that is rotated) as opposed to returning a larger rect that covers the entire region of the actual rotated Camera plane?

I'm not sure what the behavior actually should be for that or which one makes more sense, but just wanna make sure if this has been considered or not.

@DigiDuncan
Copy link
Collaborator

I'm just sanity checking that the behavior makes sense.

If the camera can rotate, should the returned Rect be accounting for the rotation(as in, returning a Rect that is rotated) as opposed to returning a larger rect that covers the entire region of the actual rotated Camera plane?

Rects can not be rotated by design, so it essentially has to for this behavior to do anything.

However, for a large amount of use cases, having this information is helpful (games where the camera does not meaningfully rotate, which is a lot of them.) But providing something if the camera rotates makes sense too.

@Cleptomania
Copy link
Member

I'm just sanity checking that the behavior makes sense.
If the camera can rotate, should the returned Rect be accounting for the rotation(as in, returning a Rect that is rotated) as opposed to returning a larger rect that covers the entire region of the actual rotated Camera plane?

Rects can not be rotated by design, so it essentially has to for this behavior to do anything.

However, for a large amount of use cases, having this information is helpful (games where the camera does not meaningfully rotate, which is a lot of them.) But providing something if the camera rotates makes sense too.

Lot of discussion about this topic happened on discord, but the TL;DR is that Rect does seem to be what other engines(namely Godot and Unity) are using for an Axis Aligned Bounding Box. There was a worry that Rect not having rotation would be misleading and not in line with other industry standard engines, but that seems to not be the case.

Regardless that's not super relevant to this PR, so this is probably fine as is.

@Cleptomania Cleptomania merged commit cf291ec into pythonarcade:development Jun 18, 2024
8 checks passed
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