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

Data Values Enhancement #2750

Closed

Conversation

Whimsyturtle
Copy link
Member

@Whimsyturtle Whimsyturtle commented Jan 8, 2020

Description

Currently, the Data Values expression (aka the Durability expression) despite being called as "Data Values" doesn't actually provide that functionality - it is only able to return / modify the durability of items.

This PR enhances the Data Values expression to actually work with data values (e.g. from blocks) and not just durability. However, due to Minecraft 1.13+ changing data values to strings instead of numbers there's a bit of complexity involved here. Listed below are the various cases that need to be handled by this new improved expression:

  • Get < 1.13 Item Durability
  • Set < 1.13 Item Durability
  • Get < 1.13 Item Data
  • Set < 1.13 Item Data
  • Get 1.13+ Item Durability
  • Set 1.13+ Item Durability
  • Get 1.13+ Item Data
  • Set 1.13+ Item Data

Target Minecraft Versions: Any
Requirements: None
Related Issues: #2736

@@ -66,53 +70,73 @@ public String getPropertyName() {
}

@Override
public Class<Short> getReturnType() {
return Short.class;
public Class<Object> getReturnType() {
Copy link
Member

Choose a reason for hiding this comment

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

making an expression return an object isn't as simple as this sadly, take a look at the ternary expression

@@ -66,53 +70,73 @@ public String getPropertyName() {
}

@Override
public Class<Short> getReturnType() {
return Short.class;
public Class<Object> getReturnType() {
Copy link
Member

Choose a reason for hiding this comment

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

you aren't actually accepting Object.class, you're accepting a Number or a String

}

@SuppressWarnings("deprecation")
@Override
public void change(final Event e, final @Nullable Object[] delta, final ChangeMode mode) {
Copy link
Member

Choose a reason for hiding this comment

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

i feel like this whole method is really confusingly written (particularly how the recursion is involved)

@@ -72,7 +72,7 @@
private class MagicBlockValues extends BlockValues {

private Material id;
short data;
private short data;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause synthetic accessors to be generated by compiler? They're somewhat problematic before Java 11 (nestmates JVM feature).

Copy link
Member Author

Choose a reason for hiding this comment

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

The itemFlags variable below is also a private primitive... is that also an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not related, or specific to primitives. It's a global thing for nested classes and private members.

If you are using Eclipse, it will show a warning when a synthetic accessor is needed to access a field. You can also enable a similar warning in the IntelliJ, from File -> Settings -> Inspections, search for "synthetic"

All fields should be package-private (no additional visibility prefix, directly type, name and obviously the semicolon) or have getters/setters and accessed by getters/setters in nested (sub) classes to not generate a synthetic accessor method.

More on synthetic access: https://stackoverflow.com/a/16226833/12308576

@bensku
Copy link
Member

bensku commented Jan 10, 2020

When working on aliases rework, I decided not to support script access NBT tags of items. We cannot guarantee their stability across versions, so usage of them should be restricted to one place - the aliases. This applies to scripters who need access to the item NBT too; aliases declared in scripts have access to exact same feature set as built-in ones do.

Besides that, the aliases system checks that the NBT data is actually valid JSON. I have no idea what happens if we just pass invalid JSON from a script to UnsafeValues (from where it goes, without validation, to NMS code).

@ShaneBeee ShaneBeee added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Mar 28, 2020
@Whimsyturtle
Copy link
Member Author

Closing this PR for now, may re-open it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants