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 #324 getViewArea was returning an wrong formatted string. #325

Merged
merged 5 commits into from
Mar 22, 2019

Conversation

source-knights
Copy link
Contributor

@source-knights source-knights commented Mar 17, 2019

This change is Reviewable

@source-knights
Copy link
Contributor Author

Fix for #324

@JamzTheMan JamzTheMan closed this Mar 18, 2019
@JamzTheMan JamzTheMan changed the base branch from merge-from-nerps to develop March 18, 2019 13:33
@JamzTheMan
Copy link
Member

We're cleaning up our Repo to follow GitFlow. I've targeted this against our Develop branch now.

@JamzTheMan JamzTheMan reopened this Mar 18, 2019
Copy link
Member

@cwisniew cwisniew left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oliver-szymanski)

@cwisniew cwisniew merged commit 8670808 into RPTools:develop Mar 22, 2019
@rkathey
Copy link
Contributor

rkathey commented Mar 23, 2019

I'm not sure the numbers add up.
getViewArea1 5 2

I ran this script

Nothing [r: getViewArea()]

0 [r: getViewArea(0)]

1 [r: getViewArea(1)]

0 json [r: getViewArea(0, "json")]

1 json [r: getViewArea(1, "json")]

0 , [r: getViewArea(0, ",")]

1 ; [r: getViewArea(1, ";")]

@Phergus
Copy link
Contributor

Phergus commented Mar 23, 2019

The numbers that are output agree with each other (for a grid size of 50 at 100% zoom) but the width and height in cells are way off for the view shown.

Too funny. It is dividing the viewport size in pixels by grid size to get the size in cells and ignoring zoom level.

@Phergus
Copy link
Contributor

Phergus commented Mar 23, 2019

It was broken this way in Nerps 1.4.5.4 as well.

@source-knights
Copy link
Contributor Author

source-knights commented Mar 23, 2019 via email

@Phergus
Copy link
Contributor

Phergus commented Mar 23, 2019

Yeah. It's not something you did. It's been like this from the beginning.

The question is what is the correct output in cell sizes for a view like above where partial cells are shown on all sides? Do you include the partials in the count or only those cells fully visible? Or do you divide the size in pixels by grid size and factor in zoom? Wouldn't think folks would want this but maybe.

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.

5 participants