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

[Bug]: Values set by token.setProperty() in GraalVM are of a different type than values set in MTScript #5152

Open
Pmofmalasia opened this issue Jan 27, 2025 · 4 comments · May be fixed by #5156
Labels

Comments

@Pmofmalasia
Copy link

Describe the Bug

When using token.setProperty() in GraalVM JS, the value set is somehow not equivalent to the same value being set in MTScript - I suspect due to variable typing. This causes issues when using getRawProperty() and likely related functions, as default values are no longer recognized as such. Opening the edit token dialog and clicking "OK" without editing any of the values reverts to expected behavior, likely because the properties are being actually updated into strings.

See this discord post if more context is needed.

To Reproduce

  1. Impersonate a token with default values for its property type. For this example, a property a5e.stat.TempHP with default value of 0 will be used.
  2. Execute [h:js.eval("let ParentToken = MapTool.tokens.getTokenByID(args[0]); ParentToken.setProperty('a5e.stat.TempHP',0);",getImpersonated())]
  3. Execute [r:getRawProperty("a5e.stat.TempHP",getImpersonated())]. This will return 0 instead of a blank string, which would be expected for a value at its default.
  4. Open the Edit Token dialog and press OK without changing any values.
  5. Execute [r:getRawProperty("a5e.stat.TempHP",getImpersonated())] again. This will now correctly return a blank string.

Expected Behaviour

Values set by token.setProperty() are able to be recognized as set to the default value in MTScript.

Screenshots

No response

MapTool Info

1.15.2, 1.16.0-beta 4

Desktop

Windows 7

Additional Context

No response

@fishface60
Copy link
Contributor

It's not a typing issue, it's that Token.setProperty is analogous to setRawProperty so unconditionally sets that value even if it's the default, while the MTScript and UI use a code path that checks whether the result is identical to the default and removes the value if it is.
Using getRawProperty is pretty much the only way to notice the distinction, though I suppose if you change defaults that would be another way.

Adding a Token.setRawProperty method that has the same behaviour as it currently works and a Token.setProperty that checks if it matches the default wouldn't be difficult, but like #4900 it invites the question whether there should be a version that instead of evaluating the default and comparing it against the value you set, that there should be a more light-weight version that treats the default as a value instead of an expression.

From your Discord message you're just attempting to reset the property to its default, which should have a Token.resetProperty binding, which would be easy and uncontroversial to add.

@Pmofmalasia
Copy link
Author

Thanks for looking into this!

Adding a Token.setRawProperty method that has the same behaviour as it currently works and a Token.setProperty that checks if it matches the default wouldn't be difficult, but like #4900 it invites the question whether there should be a version that instead of evaluating the default and comparing it against the value you set, that there should be a more light-weight version that treats the default as a value instead of an expression.

I think the paradigm you've set up in #4900 makes sense to expand into setProperty, as you've described here. To be honest the whole Raw vs. non-Raw property paradigm feels a bit kludgy to me, but I don't think it can really be modified without destroying backwards compatibility So having setProperty be the default, less irritating version seems good.

From your Discord message you're just attempting to reset the property to its default, which should have a Token.resetProperty binding, which would be easy and uncontroversial to add.

This would be good as well, but while this is my current use case I can foresee issues still arising when generally setting a value that happens to be the default - using Temp HP as an example again, would happen commonly when damaging a creature and removing all of its Temp HP.

@fishface60
Copy link
Contributor

Adding a Token.setRawProperty method that has the same behaviour as it currently works and a Token.setProperty that checks if it matches the default wouldn't be difficult, but like #4900 it invites the question whether there should be a version that instead of evaluating the default and comparing it against the value you set, that there should be a more light-weight version that treats the default as a value instead of an expression.

I think the paradigm you've set up in #4900 makes sense to expand into setProperty, as you've described here. To be honest the whole Raw vs. non-Raw property paradigm feels a bit kludgy to me, but I don't think it can really be modified without destroying backwards compatibility So having setProperty be the default, less irritating version seems good.

I'm not a fan of raw vs non-raw either, but that's because I don't like the behaviour of resetting a property if the value you set it to is the same as the default.

It means you've got to evaluate the default expression every time you set a property and you might intentionally want a value to be set to a value that happens to be the default but conceptually isn't.

e.g. If I have a Hero token type that has Strength 10, and I have a given token with Strength 10 because they started with 10, were cursed for a -1 and have a magic sword that gives +1 I would like that to still be 10 after I decide that late on in the campaign new heroes need a buff and I change the token type to have default Strength 12.

I would prefer that resetting to default were an explicit step, there is a macro function to do it and I could add a JavaScript function.
I would prefer that the API is:

  • Token.setProperty - behaves as setRawProperty
  • Token.getPropertyDefault - returns the value of the default
  • Token.getPropertyDefaultEvaluated - evaluates the default expression and returns that value
  • Token.resetProperty - removes the value from the token so it falls back to the default.

This way you could implement resetting to default however you like, without evaluating the content.

However this is a pipe dream if every time you open the token editor and save it's going to evaluate the default and remove the value if it's the same as the default.

Since that is a much wider change that'd have compatibility issues I suppose I'll settle for:

  • Token.setProperty - behaves as setProperty, evaluating the default and resetting the property if the new value is the same as the default.
  • Token.setRawProperty - sets the value directly without checking against the default
  • Token.getPropertyDefault - returns the value of the default
  • Token.getPropertyDefaultEvaluated - evaluates the default expression and returns that value
  • Token.resetProperty - removes the value from the token so it falls back to the default.

@fishface60
Copy link
Contributor

Okay, now that I've actually had time to look at it, it's the Token Editor dialog that's different here.

Setting values by name in impersonation scope sets the value. The MTScript setProperty macro function sets the value in the token's property mapping the same as the new JavaScript Token.setProperty.

It's only the edit dialog that resets token property values if they equal their default.

public void applyTo(Token token) {
for (TokenProperty property : getPropertyList()) {
String value = getPropertyMap().get(property.getName());
if (property.getDefaultValue() != null && property.getDefaultValue().equals(value)) {
token.setProperty(property.getName(), null); // Clear original value
continue;
}
token.setProperty(property.getName(), value);
}
}

Seems we were both wrong about how it works. Checking back through the logs it's always been the case that macros set the values directly and you need to use resetProperty if you want it to clear it to use the default, but the token editor wipes it, which doesn't seem like a useful way to work.

@fishface60 fishface60 linked a pull request Jan 30, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants