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

[Feature] Typesafe datatags #669

Merged
merged 12 commits into from
Jul 25, 2024
Merged

Conversation

DerToaster98
Copy link
Contributor

@DerToaster98 DerToaster98 commented Jul 21, 2024

Describe in detail what your pull request accomplishes

This PR adds typesafe datatags. Datatags work similar to craftfile properties in that they need to be registered before using them. Their value can be accessed via methods on the craft directly.

Checklist

  • Unit tests
  • Proper internationalization
  • Tested

TODO list:

  • API classes
  • Access methods
  • Calling update methods of the value in the respective spots
    • onRotate
    • onSwitchWorld
    • onRelease
    • onAssembly
    • onTranslate

Related PRs:

Copy link
Contributor

@TylerS1066 TylerS1066 left a comment

Choose a reason for hiding this comment

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

Looking at this PR, I think there's a difference in what we view craft data tags to be. I view them as being type-safe data associated with a craft by a namespaced key.

The first and foremost of this would be the contacts system, I envision ContactsManager would have the following line:
public static final NamespacedKey CONTACTS = new NamespacedKey("movecraft", "contacts");
Then anywhere you need to get the contacts from a craft you would run the following:
craft.getObjectTag(ContactsManager.CONTACTS) to return an Object. We'll have to decide what types we want explicit type safety on (since it's a lot of duplicate code for each), but here's my initial list that should cover most data in crafts:

  • boolean
  • double
  • long
  • String
  • Object (like craft properties, type safety will be the responsibility of the user)

@DerToaster98
Copy link
Contributor Author

Looking at this PR, I think there's a difference in what we view craft data tags to be. I view them as being type-safe data associated with a craft by a namespaced key.

The first and foremost of this would be the contacts system, I envision ContactsManager would have the following line: public static final NamespacedKey CONTACTS = new NamespacedKey("movecraft", "contacts"); Then anywhere you need to get the contacts from a craft you would run the following: craft.getObjectTag(ContactsManager.CONTACTS) to return an Object. We'll have to decide what types we want explicit type safety on (since it's a lot of duplicate code for each), but here's my initial list that should cover most data in crafts:

* boolean

* double

* long

* String

* Object (like craft properties, type safety will be the responsibility of the user)

Well what i expect is this:

CraftDataTagKey<Contacts> CONTACTS_KEY = register("contacts", Contacts::new)

If i want the contacts, i can just do
Contacts contacts = Craft.GetDataTag(CONTACTS_KEY)

That way, no cast is necessary and i can directly access it.
I don't see dedicated types for bool, double, etc as necessary, for those one can just use the wrapper classes. Also havign dedicated classes for that in my opinion just adds bloat code. What i can live with is additional methods for the primitives and one getObject method that works like my example above (aka the current implementation). Otherwise i agree with your definition of craft data tags, for me it is just additional data that a craft drags along for itself, in addition it would be neat if that data can directly react to some events like the craft being piloted or moved.

@TylerS1066
Copy link
Contributor

That's an interesting approach and I like the implicit type safety built in. Is it possible we could change the supplier to a Function<Craft, ...> that way you can calculate the value based on the craft?

@DerToaster98
Copy link
Contributor Author

That's an interesting approach and I like the implicit type safety built in. Is it possible we could change the supplier to a Function<Craft, ...> that way you can calculate the value based on the craft?

Yes, sure, that's a good idea!

@TylerS1066
Copy link
Contributor

I've tried out this system to implement contacts and I much prefer it over the map it used to contain! Once this PR is cleaned up and merged I'll rebase these changes and merge them as well.

@DerToaster98
Copy link
Contributor Author

I've tried out this system to implement contacts and I much prefer it over the map it used to contain! Once this PR is cleaned up and merged I'll rebase these changes and merge them as well.

I was also planning to use this for various changes i want to make to movecraft combat (mainly a overall rework of directors), glad you like it.

…into typesafe-datatags

# Conflicts:
#	api/src/main/java/net/countercraft/movecraft/craft/datatag/CraftDataTagContainer.java
@DerToaster98
Copy link
Contributor Author

Removed the unnecessary interface, that interface is only really useful if we want to directly support datatypes that can directly react to events

@DerToaster98 DerToaster98 requested a review from TylerS1066 July 24, 2024 16:51
Copy link
Contributor

@TylerS1066 TylerS1066 left a comment

Choose a reason for hiding this comment

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

Only thing left is to clear up the diff with the following files:

  • Movecraft/src/main/java/net/countercraft/movecraft/craft/CruiseOnPilotSubCraft.java
  • Movecraft/src/main/java/net/countercraft/movecraft/craft/SinkingCraftImpl.java
  • Movecraft/src/main/java/net/countercraft/movecraft/craft/SubCraftImpl.java
  • Movecraft/src/main/java/net/countercraft/movecraft/craft/SubcraftRotateCraft.java

@DerToaster98
Copy link
Contributor Author

Done

@TylerS1066 TylerS1066 merged commit a7aa511 into APDevTeam:main Jul 25, 2024
1 check passed
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.

2 participants