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

Restrict portrait in overlay to maximum image size when resizing with SwingUtil.constrainTo #1583

Closed
nmeier opened this issue Apr 17, 2020 · 15 comments
Assignees
Labels
deferred Kept for a future version feature Adding functionality that adds value

Comments

@nmeier
Copy link
Contributor

nmeier commented Apr 17, 2020

Is your feature request related to a problem? Please describe.

Currently the statsheet overlay with portrait turned on is forced to a static value from AppPreferences.getPortraitSize. I've bumped up that setting to a large value so I can get a decent sized portrait to show up (larger than 175). This works for those pics that are large enough:

image

Smaller images get stretched to my new limit of 600 though, which leads to ugly stretching.

image

Describe the solution you'd like

I think it would be better if the portrait doesn't get stretched beyond what its native resolution is. In my 2nd case above

image

Describe alternatives you've considered

There could be another option in the preferences but that dialog seems overloaded already.

Additional context

I have a fix ready and figuring out how to make that a pull request.

@Phergus Phergus added the feature Adding functionality that adds value label Apr 17, 2020
@Phergus
Copy link
Contributor

Phergus commented Apr 17, 2020

Preferences does have a size setting for the portrait.
Screenshot 2020-04-16 19 35 14

In any case, it definitely shouldn't scale it past native size.

@Phergus
Copy link
Contributor

Phergus commented Apr 17, 2020

Before submitting a PR a couple notes.

  • Create a local branch off of develop to do your work.
  • When you submit your PR target develop and make sure you have Allow edits from maintainers. checked.

Read over these guidelines:
https://github.com/RPTools/maptool/blob/master/doc/Code_Style_and_Guidelines.md

nmeier added a commit to nmeier/maptool that referenced this issue Apr 17, 2020
… grow beyond its natural size.

Make portrait in overlay/statsheet not grow beyond image size.

Fixes RPTools#1583
@JamzTheMan
Copy link
Member

So. If I'm on a hidpi monitor and my gm used portraits of low resolution, you are saying I can not scale those up so I can see them??

@Merudo
Copy link
Member

Merudo commented Apr 17, 2020

Maybe make it a per-token option? The picture of the token could either "scale-down" to fit the specified size (lowers its size until it fits inside the panel, but not enlarge it). Or , it could be forced to take the exact size (current behavior).

@Phergus
Copy link
Contributor

Phergus commented Apr 17, 2020

If it was made a Token option, the Jamz would still lack control over the portrait size on his display.

Could also be an preference option to go along with the setting for portrait size.

  • Enlarge undersized Portraits.

I'm guessing that folks will then want:

  • Use native size for Portraits.

@nmeier
Copy link
Contributor Author

nmeier commented Apr 17, 2020

There could be more options but one could argue the GM should have sufficient resolution for hi def players.

How about two boxes Statsheet portrait size [Min] and [Max]. Pictures are scaled at min to what the hiresolution player has, and restricted by max. One could fit two text boxes into the space where there's currently one.

@JamzTheMan
Copy link
Member

JamzTheMan commented Apr 17, 2020

Right now Statsheet Portrait Size is used as a set size and 0 removes the image.

How about we allow 200+ which would set the portrait to 200 OR what ever the images size is

or maybe allow min(200) or max(200) as values to set the portrait equal to the image size with a min or max of x?

or maybe 100-300 uses portrait image size with a min of 100 and max of 300?

Lots of options...

@JamzTheMan
Copy link
Member

There could be more options but one could argue the GM should have sufficient resolution for hi def players.

This same argument could be used against your example. It wouldn't be "ugly stretching" if the GM supplied appropriate 600x600+ hidpi images.

@nmeier
Copy link
Contributor Author

nmeier commented Apr 20, 2020

Right now Statsheet Portrait Size is used as a set size and 0 removes the image.

Is 0 needed as a value? There's also a setting for having to use shift to reveal the portrait. One could use the number as before for a target sizE to scale to and use 0 as use-picture-native-size.

A range sounds easy enough though, could make this 64-200 as suggested yes

@Phergus
Copy link
Contributor

Phergus commented Apr 26, 2020

This still needs to be resolved. Here are the options:

  1. Revert change - issue will be pushed to next release
  2. Change Prefs to accept two values in current size field. As it isn't actually a "range", would prefer to see something like 80:250 but either way it is probably prone to user errors.
  3. Change Prefs to have both Min & Max size fields

I'll need a PR doing either option 2 or 3 in the next couple days or we default to option 1.

@nmeier
Copy link
Contributor Author

nmeier commented Apr 27, 2020

Ultimately I think the range is too complicated. Vote to stay with #1 and stick with the old behavior, right 50% of the time.

@nmeier
Copy link
Contributor Author

nmeier commented Apr 27, 2020

btw realized why the scaled images bug me - the handout and the token draw doesn't scale for quality but for speed. See the difference in handout and a couple of tokens - RIGHT as per current maptool, LEFT when I tweak the graphics2d settings

image

There's code all over the place that modifies graphics2d - looking into whether there's a good place to tweak the appropriate hint that's currently missing

[edit:fixed left/right]

@Phergus
Copy link
Contributor

Phergus commented Apr 27, 2020

@nmeier Do you have Display Scaling enabled in Windows?

@nmeier
Copy link
Contributor Author

nmeier commented Apr 27, 2020

@Phergus don't have that on. Confirmed the issue is the rendering hints that are not applied in token rendering and the portrait floating view #1709

For this here, let's roll back. Don't think we need to keep this request for a flip in sizing behavior to stay open.

@Phergus
Copy link
Contributor

Phergus commented Apr 27, 2020

Roger that. This can be explored later after the move to Java 14 and in the context of some of the newer features such as overlays.

@Phergus Phergus added the deferred Kept for a future version label Apr 27, 2020
@Phergus Phergus closed this as completed Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred Kept for a future version feature Adding functionality that adds value
Projects
None yet
Development

No branches or pull requests

4 participants