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

@co012/position and bounds #33

Merged
merged 18 commits into from
May 14, 2022
Merged

@co012/position and bounds #33

merged 18 commits into from
May 14, 2022

Conversation

co012
Copy link
Collaborator

@co012 co012 commented May 5, 2022

Description

Checklist

  • Included code to test these changes
  • Updated Jira

@co012 co012 marked this pull request as ready for review May 13, 2022 19:38
@co012 co012 requested review from kkafar and mhawryluk May 13, 2022 19:38
Copy link
Collaborator

@mhawryluk mhawryluk left a comment

Choose a reason for hiding this comment

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

Great, a lot of good changes as far as I can tell. I do have a few comments though!

src/main/java/io/rpg/model/location/LocationModel.java Outdated Show resolved Hide resolved
Comment on lines 140 to 143
private Point2D getBoundPosition(Point2D pos) {
double x = Math.max(0, Math.min(bounds.getX(), pos.getX()));
double y = Math.max(0, Math.min(bounds.getY(), pos.getY()));
return new Point2D(x, y);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The behavior of bounds is inconsistent - top and left bounds stop the player immediately, while the right and bottom ones let them cross up to half of the player's size.

I think there are two equally good options from here:

  • change from 0 to -0.5 in Math.max
    or:
  • change bounds from (9.5, 9.5) to (9, 9), or in general when read from config: (width-1, height-1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You, are right. I'll go with your first suggestion

import java.lang.reflect.Field;
import java.util.LinkedHashSet;
import java.util.Optional;
import java.util.Set;
import javafx.beans.property.ObjectProperty;
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused import

co012 and others added 2 commits May 14, 2022 18:57
Co-authored-by: Marcin Hawryluk <70582973+mhawryluk@users.noreply.github.com>
@co012 co012 merged commit 5d17086 into master May 14, 2022
@co012 co012 deleted the @co012/position_and_bounds branch May 14, 2022 19:59
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.

2 participants