Token class updates to accommodate new layout features #5104
Conversation
clean up some property names, e.g. isFlippedIso to flippedIso so that getter becomes isFlippedIso(). Unnecessary Token removed from property tokenOpacity. Now just opacity. Reordered to keep layout props together. Added new layout prop imageRotation Updated tokenLayoutProp macro functions
bubblobill
added
feature
|
Yay, it runs, and I am reminded why methods should be properly named the first time. |
src/main/java/net/rptools/maptool/client/ui/token/dialog/edit/EditTokenDialog.java
Outdated
Show resolved
Hide resolved
src/main/java/net/rptools/maptool/client/ui/token/dialog/edit/TokenLayoutPanelHelper.java
Outdated
Show resolved
Hide resolved
src/main/java/net/rptools/maptool/client/ui/token/dialog/edit/TokenLayoutPanelHelper.java
Outdated
Show resolved
Hide resolved
…ented code. Commented code removed
Token drawing moved to TokenRenderer Facing-arrow painting moved to FacingArrowRenderer Some functionality moved to ImageUtils
Remove old layout panel
|
In order to get tokens to paint properly I had to mess with ZoneRenderer. The renderTokens() code in ZR is a nightmare of repetitious code and lots of fragile non-optimal stuff. Since ZR needs major splitting anyway, I created TokenRenderer. At present it only deals with painting tokens and I left all the weird stuff I don't understand properly behind. No idea if it is faster, but image operations are performed in the following sequence with the intention of maintaining the best quality; Flip X/Y -> Flip Iso -> Scale[X, Y, XY, Footprint, and Zoom (all at once to prevent multiple scaling lossiness)] -> Rotate. Image caches are cleared on change of zoom. Not sure if it is necessary but it seemed safer. There are also problems with token bounds and locations. The whole everything works this way except for squares annoys the crap out of me as Everything I do is based on the token's actual position. Images are transformed such that the origin lies at their centre +/- any anchor. Translate the graphics object to the token position, scale and rotate as required and everything is easy. This is what happens if halos are drawn on offset images. Both tokens are StG. This is what the shark should look like. Since it has always been the case I didn't mess with it. But if all this gets through then I am mightily inclined to change it. |
Icons embedded
|
I took this for a test and found a bug regarding halos. A token with no halo is shown as having the "white" halo when the menu is opened. |
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| /* Utility class for useful mathematical methods */ | ||
| public class MathUtil { |
There was a problem hiding this comment.
We can do without most of this class as the methods are either unused or can be simplified. I'll leave specific comments below.
| private static final Logger log = LogManager.getLogger(MathUtil.class); | ||
|
|
||
| /** | ||
| * Faster version of absolute for integers |
There was a problem hiding this comment.
Any proof of this claim? Math.abs() calls are always inlined with platform-specific instructions, so there's no branching concerns with it.
Anyways, this method isn't used and can be removed.
| } | ||
|
|
||
| /** | ||
| * Faster version of absolute |
There was a problem hiding this comment.
I guarantee this is slower than Math.abs(). Plus this method isn't used.
| * @return the equivalent value of valueToMap in the target range | ||
| * @param <T> | ||
| */ | ||
| public static <T extends Number> T mapToRange( |
There was a problem hiding this comment.
The generics don't really gain us anything here since internally it has to check float vs double anyways. I.e., an overload would do just as well.
But it can also just be replaced with a purely double version. The one caller that uses float can cast the result.
| ((mapValue.doubleValue() - inMin.doubleValue()) | ||
| * (outMax.doubleValue() - outMin.doubleValue()) | ||
| / (inMax.doubleValue() - inMin.doubleValue()) | ||
| + outMin.doubleValue()); |
There was a problem hiding this comment.
This kind of lerp implementation tends to be inaccurate when valueToMap == in_max, due to floating point shenanigans. An alternative with accurate endpoints would be:
var t = (valueToMap - in_min) / (in_max - in_min);
result = (1 - t) * out_min + t * out_max;| * @param highBound | ||
| * @return | ||
| */ | ||
| public static int constrainInt(int value, int lowBound, int highBound) { |
There was a problem hiding this comment.
This method is the same as Math.clamp(long, int, int) and can be removed in favour of that.
| * @return | ||
| * @param <T> | ||
| */ | ||
| public static <T extends Number> T constrainNumber(T value, T lowBound, T highBound) { |
There was a problem hiding this comment.
This method is equivalent to Math.clamp(double, double, double) and can be removed.
| return d; | ||
| } | ||
|
|
||
| public static boolean isDouble(Object o) { |
There was a problem hiding this comment.
With the other proposed simplifications, isDouble(), isFloat() and isNumber() are not needed.
| * @return truncated double value | ||
| */ | ||
| public static double doublePrecision(double value, int decimalPlaces) { | ||
| double d = Double.parseDouble(String.format("%." + decimalPlaces + "f", value)); |
There was a problem hiding this comment.
This is fine for the UI usage, but outside of that will be a pretty slow implementation. Can use this instead:
double divisor = Math.pow(10, -decimalPlaces);
double d = divisor * Math.round(value / divisor);
|
|
||
| private Integer visionOverlayColorValue; | ||
| private transient Color visionOverlayColor; | ||
|
|
||
| // Jamz: allow token alpha channel modification | ||
| private @Nonnull Float tokenOpacity = 1.0f; | ||
| private @Nonnull Float opacity = 1.0f; |
There was a problem hiding this comment.
Can't rename fields or token settings in existing campaigns will be lost (applies to opacity, isFlippedX, isFlippedY, isFlippedIso).
It's fine that the accessor methods or DTO fields are renamed, just the model fields need to remain the same.
|
Got partyway through reviewing. Will pick it up again later today. The feature is looking quite good. The new layout functionality feels very nice to use. |
| String rotation = String.valueOf(token.getImageRotation()); | ||
| String scaleX = String.valueOf(token.getScaleX()); | ||
| String scaleY = String.valueOf(token.getScaleY()); | ||
| String footprintScaleValue = |
There was a problem hiding this comment.
These should all be left as numeric values. Otherwise the JSON will contain a bunch of strings that will then require further parsing by the caller.
| sb.append("scaleX=" + scaleX + delim); | ||
| sb.append("scaleY=" + scaleY + delim); | ||
| sb.append("footprintScale=" + footprintScaleValue); | ||
| return sb.toString(); |
There was a problem hiding this comment.
Since you're using a StringBuilder anyways, might as well avoid the (expensive) string concats and chain the appends. E.g.,
sb.append("scale=").append(scale).append(delim);| /* | ||
| * setExtendedTokenLayoutProps(StrProp/JSON Object, token: currentToken(), mapName = current map) | ||
| */ | ||
| if (functionName.equalsIgnoreCase("setExtendedTokenLayoutProps")) { |
There was a problem hiding this comment.
Anyway this could be folded into setTokenLayoutProps()? The set of options parallels the output of getTokenLayoutProps() and so would be easier for macro writers to discover.
There was a problem hiding this comment.
I'm not sure what changed, but I'm finding that "Flip > Isometric Plane" doesn't result in correct rendering. The token is typically rendered flat, but once it starts being dragged it becomes iso.
| try { | ||
| return ImageUtil.scaleBufferedImage(img, outputWidth, outputHeight); | ||
| } catch (Exception e) { | ||
| log.debug(e.getLocalizedMessage(), e); |
There was a problem hiding this comment.
Is there an actual error that we should be concerned about here? At a glance, I don't think the ResampleOp will throw anything in our case.
If there is a worry, this should be a log.error(), or at least a log.warn().
| return ImageUtil.scaleBufferedImage( | ||
| img, (int) Math.ceil(b.width * zoom), (int) Math.ceil(b.height * zoom)); | ||
| } catch (Exception e) { | ||
| log.debug(e.getLocalizedMessage(), e); |
There was a problem hiding this comment.
Same as above, either don't handle the error or make it log.error().
| * @param token The token containing the flip-states | ||
| * @return A modified image, or the original image if no processing performed. | ||
| */ | ||
| public static BufferedImage flipTokenImage(BufferedImage image, Token token) { |
There was a problem hiding this comment.
This method isn't used.
| return bytesToImage(dataStream.toByteArray(), image); | ||
| } | ||
|
|
||
| public static double getIsoFigureHeightOffset(Token token, Rectangle2D footprintBounds) { |
There was a problem hiding this comment.
getIsoFigureHeightOffset() and getIsoFigureScaleFactor() seem like they would be more natural in the Token class as they don't really have to do with image manipulation even though they are fed into token image rendering.
resolves #5103
Progresses #5096
Progresses #3691
Includes most of #5097
Description of the Change
New property
New property macro functions
Refactoring
Possible Drawbacks
none
Documentation Notes
n/a
Release Notes
n/a
This change is