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

Unified Data API #542

Closed
wants to merge 1 commit into from
Closed

Unified Data API #542

wants to merge 1 commit into from

Conversation

gabizou
Copy link
Member

@gabizou gabizou commented Apr 4, 2015

Data API Proposal

It certainly has been a while since I've last made a rather large API PR It's been three weeks since my last major API PR, however, this is the culmination of some major features being created and designed before this proposal could come to fruition. Without further ado, I present the Data API.

What is DataAPI?

Quite simply, it's an API centered around anything holding mutable data being a DataHolder. The front facing view of DataHolder doesn't have any guarantees about the data possibly being held by the holder.

So... what's the point?

Well, to put it bluntly, Entities, TileEntities, Blocks, and Items all become similar DataHolders in that they all are "basic" objects that can have extraneous data associated with them.

But you already wrote Items API!

Yeah, and? It was a pretty sweet API, wasn't it? As an idea, Items API was simply a stepping stone towards this overall Data API to cover more than just Items. The same structure that Items API had is now applicable to Entities, Blocks, and TileEntities!

Ok but why the change?

Sadly, there were several limitations of what could be represented by ItemData. First, ItemData could not translate anything about an Entity or its data. Second, ItemData couldn't represent TileEntityData without blatantly defining a method such as TileEntityData<?> getTileEntityData() which, let's be honest, is almost as useless as saying "Hey! I'll give you an Object but you have to check what type of object it is first!". Lastly, it was impossible to define various information about an Entity within an ItemStack without creating some sort of EntityData.

But before you ask more questions or make comments, let's actually review the new structure of Data.

New Beginnings for DataHolder

DataHolder was a simple interface that has existed for quite some time now, having a single method: <T> Optional<T> getData(Class<T> dataClass). Great! Well, we learned to actually use it in a few places, but it has its own set of limitations. Namely, we can't modify a DataHolder. To change this, DataManipulator was created with a few simple methods to make it usable:

  • clear()::void
  • from(DataHolder)::DataManipulator
  • from(DataContainer)::DataManipulator

All is good. But what about overrides? I mean, when we have a DisplayNameData, we usually want to be able to do the following

displayName.setName("[MySuperPlugin]" + displayName.from(myItemStack).getName());`

So, what do we do? _Add Generics!_

DataManipulator became abstract enough that DataHolder could effectively house all getters and setters for data! What used to reside in ItemStack now resides in DataHolder! And since DataHolder is a super interface, well, we have the capability of representing data from almost any DataHolder source and sending that data to any compatible DataHolder!

Tidying Up

Because DataHolder was a super interface, we gained the ability to make changes to DataManipulators and set them on more than a single type of DataHolder. However, we still had the problem of actually representing data that hasn't been represented before, namely, data from Entity and data from previously known as BlockLoc.

Getting this done required lots of contemplation about every Entity interface and the data represented by it and what "function" that interface had. In reality, many interfaces were kept mostly for the sake of being able to comply with the underlying structure of entities in Minecraft.

I didn't really like that bit. Especially since something as simple as OcelotType is in itself representing some sub type of an Ocelot entity. So, what could be done? Well, a compromise:

I came to the conclusion that any Entity interfaces that needed to exist for their functionality and mixing compatibility with implementation would stay. Their data accessor methods however, would now reside into categorized DataManipulators such that we now have the following possible:

Entity randomEntity = randomEntityEvent.getEntity();
Optional<HorseData> optional = randomEntity.getData(HorseData.class);
if (optional.isPresent()) {
    HorseData data = optional.get();
    data.setVariant(HorseVariants.SKELETON_HORSE);
    randomEntity.offer(data);
}
DisplayNameData data = randomEntity.getOrCreate(DisplayNameData.class).get();
data.setDisplayName(Texts.of(TextColors.AQUA, TextStyles.BOLD, "SKELETOR"));
randomEntity.offer(data);

One thing while writing these various DataManipulators for entities is that there's quite a bit of overlap with some specific ItemData and EntityData. Namely, the overlaps involved things like PotionEffects, DisplayName, DyeColor, etc. So, I made sure that these overlaps didn't exist.

Finally, came the final step in this challenge: Representing BlockState information with Data API...

It took a while, but the end result is that BlockState can be considered an immutable DataHolder where it has an ImmutableCollection<? extends DataManipulator<?>> where these manipulators are mutable, but again, the underlying BlockState can not be changed.

Finally, with the BlockState change, representing various BlockProperties was doable! In came the various DataManipulators for block information, like, RailDirectionData, and TreeTypeData, but something that I further realized is that there is even duplication across almost all DataManipulators that hold a single value and allow the developer to change that single value based on some predetermined bound of either the object type or a primitive type boundary.

What ended up being created is an exhaustive list of all types of DataManipulators that now not only represent Items, Entities, Blocks, and TileEntities, but DataManipulators that can be interchanged with other DataHolders!

Just imagine it this way: Remember how I demonstrated that you can set an inventory on a chest, even though the chest is in the form of an ItemStack? Apply the same idea to the four big DataHolders: BlockData can be retrieved from an ItemStack, and an ItemStack can set Entity data, and an Entity can be manipulated by a TileEntity just the same! Pretty freaking awesome, isn't it?

With PR, we've gained the following features:

  • Interchangeable DataManipulators that can be used to manipulate data on any Item, Block, Entity, and TileEntity
  • Removed all remnant getByteData methods and notions to represent "magic values"
  • Provide a step forward for plugin developers to start using data more intelligently
  • Potentially future proof the core Data API for future Minecraft versions to come
  • Unify Data Manipulation across multiple APIs with minimal maintenance overhead

Being that this PR will break several API's when merged, an implementation PR is being prepared for merging such that Sponge Implementation can update with minimal wait time during the API breakage.

@gabizou gabizou changed the title Unified Data API Unified Data API (still working on javadocs) Apr 4, 2015
@gabizou gabizou changed the title Unified Data API (still working on javadocs) [WIP] Unified Data API (still working on javadocs) Apr 4, 2015
@gabizou gabizou added this to the 2.0-Release milestone Apr 5, 2015
@ST-DDT
Copy link
Member

ST-DDT commented Apr 5, 2015

@gabizou Pretty impressive.

I did not have time to read all the changes yet, but i will do this once the javadocs are changed.
But i have noticed that the Player's compass target is not get/set-able in the API (now and before), it is possible as proven by Bukkit. Am i right this will/would be implemented as a data class as well? (Will you add it here as well or does that needs discussion and thus be better off in its own issue?)

I have seen the EntityState and similar classes have been removed, does that mean, that if i grab the entities' data they are no longer live? Is it for Entity, TileEntity, Block and Item all the same?
Example: Every change to Entity is live, but the data objects returned by getData() are some kind of copy i can modify and store somewhere (even in a file) without affecting the entity (unless i set the modifed data directly back to the entity). Are ALL data objects safe/detached or may they cause async execution errors?

@gabizou
Copy link
Member Author

gabizou commented Apr 6, 2015

@gabizou Pretty impressive.

I did not have time to read all the changes yet, but i will do this once the javadocs are changed.
But i have noticed that the Player's compass target is not get/set-able in the API (now and before), it is possible as proven by Bukkit. Am i right this will/would be implemented as a data class as well? (Will you add it here as well or does that needs discussion and thus be better off in its own issue?)

I have seen the EntityState and similar classes have been removed, does that mean, that if i grab the entities' data they are no longer live? Is it for Entity, TileEntity, Block and Item all the same?
Example: Every change to Entity is live, but the data objects returned by getData() are some kind of copy i can modify and store somewhere (even in a file) without affecting the entity (unless i set the modifed data directly back to the entity). Are ALL data objects safe/detached or may they cause async execution errors?

Short answer: Yes.

Long answer: Yes it would be a data class. No I won't add it here, yes it will need discussion and added in a later PR. Yes data is no longer live, just like getting data from any of the data accessors that are currently available in an entity. Yes, all data is now joined in a unified format, all are grabbed and not retaining any form of synchronization with the owner of the data. Almost all objects are safe. The only one that is not always safe is anything to do with Inventory.

@gabizou gabizou mentioned this pull request Apr 7, 2015
@gabizou gabizou force-pushed the feature/data branch 2 times, most recently from 2e08789 to 0fa42e9 Compare April 7, 2015 20:50
@gabizou gabizou force-pushed the feature/data branch 8 times, most recently from c3aa24a to c40490e Compare April 8, 2015 04:17
@gabizou gabizou changed the title [WIP] Unified Data API (still working on javadocs) Unified Data API Apr 8, 2015
…unified Data API.

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>
@Jameshobbs21
Copy link

I am curious as to how modular this system is in terms of adding custom data.

For instance, would it be possible to add a special type of modifier to an item stack to apply some special effect that is defined.

Another example could be adding a custom ability to a zombie entity.

These custom data APIs would essentially have to have callback functions to provide the proper functionality. I would imagine that a custom data type for a weapon would implement some interface that provides a method for when that weapon is used (i.e. weapon was used to attack some entity), which then would implement said method with the custom special effects.

I really love the idea and I think it is intuitive, but just curious how far you think we could push this to add custom data effects.

The benefit of adding these custom attributes is being able to spawn an item stack or entity and pushing these custom data attributes onto the item which will then automatically handle the events to provide the functionality.

@ryantheleach
Copy link
Contributor

Yes it can, but Wouldn't it just be easier to have the plugin listen to the events, then act on them or modify the events based on the custom data present?

Making the custom DataManipulator do it just seems strange.

@Jameshobbs21
Copy link

That is essentially how I handle custom data in bukkit, which can get tedious as I have to keep track of which entity uuids I want to have custom abilities.

Would be nice to just bind the custom data to the entity I am spawning and not have to worry about when that specific entity does the event. (So another example could be one set of zombies have one ability, whereas another set are completely different)

Perhaps the entity example is more for customizing the AI for specific mobs, but it really depends on how we define data. Same goes for item custom data as I want to modify how a specific item behaves when it is being used (however I dont want all items of the same type to behave like this, it would be more of a unique item that spawns rarely).

@ryantheleach
Copy link
Contributor

Except you don't need to keep track of UUID's because you can tag it with the custom data, and it may be persistent.

@Jameshobbs21
Copy link

Thinking more in terms of what makes up an entity and what defines the entity.

Something more along the lines of this: https://forums.spongepowered.org/t/component-system-and-minecraft/76/23

Where you define the custom data and attach them. For me it feels intuitive and simple to implement for plugin developers compared to having all of my custom data handling be done in events.

Using the current proposed system, how to identify which custom data is applied to a particular entity?

Given three types: CustomAttack1, CustomAttack2, CustomAttack3

Would I need to specifically check if each of these custom data types are present for every entity that attacks? I think this would not be efficient. And a better solution would be to iterate over every custom attack data attached to each entity that attacks. If there is no custom attack data attached, then nothing happens, otherwise the appropriate method hook is called.

Example code:

interface CustomAttack
{
   // target is the entity that is being attacked, returns damage dealt
   double attack(Entity target, double damage);
}

class CustomAttack1 implements CustomAttack
{
    double multi;
    public CustomAttack1(double multiplier)
    { multi = multiplier; }

   @Override
   public double attack(Entity target, double damage)
   {
       return damage * multi;
   }
}

...

Entity newEntity = Entity.spawn(Zombie.class);
newEntity.attach(new CustomAttack1(2.0);

I hope you see the benefits of this system and trying to incorporate things at a lower level.

@Soren025
Copy link

@ryantheleach The last thing you want is to register an event handler for each entity. Especially since the only way to unregester them last time I checked is to unregester all event handlers for a given plugin.

Thus why you would need to keep track of the uuids within a single event handler.

@Soren025
Copy link

@Jameshobbs21 I rather like the idea of DataManipulator having callbacks based on the type of DataHolder. It would create a really similar workflow to unity if you think of (DataHolder -> GameObject) and (DataManipulator -> Component)

@Soren025
Copy link

Would also take care of my wish for #447

@ryantheleach
Copy link
Contributor

You wouldn't need one for each entity, you would only need a single listener for all entities.

listenForInterestingEntityEvent(entityEvent e){
  Optional<MyCustomDataManipulator> myData = e.getEntity().getData(MyCustomDataManipulator.class);
  if(myData.isPresent()){
    doCustomBehavior(myData.get());
  }
}

In addition, all the entity component systems I've seen, the component is just data that you need for rapid processing in a system, and/or a reference to where you can find less used data.

The CustomDataManipulator would take the role of the Component, and the rest of the plugin the system.

@Soren025
Copy link

@ryantheleach Unity Doesn't really follow that so its not really an ECS by defination. But it works and can work here too.

@ryantheleach
Copy link
Contributor

Unity isn't the only ECS system in existence.

@ST-DDT
Copy link
Member

ST-DDT commented May 16, 2015

I like the idea of binding specific attack patterns/effects to an entity.
However there are some points/concerns I have with the suggested system.
If it uses the DataAPI with a callback then it does not persist. At least not without some special restore code. This does not occur if you use a simple data tag and check for it during events.
More importantly you cannot do very much in this Attack object. You can cast some particles and sound and define the damage, but you cannot define attack speed, range, passive weapon effects ...
These all are features for a AI API I would like to see, so my mayor concern with this proposal is that it may interfere with the AI API because it implements a part of it without taking care of the rest.

So i would suggest using @ryantheleach's suggestion with data tag and event handler to solve your issue until the full blown AI API is there.

@Jameshobbs21
Copy link

@ST-DDT I see what you mean, and agree that it fits better with the entity AI API.

Would this apply to item stacks as well? For instance could we define AI for a particular item so that whenever an attack occurs it does some form of AOE damage. Or perhaps the item increases some modifiers for a player when that item becomes equipped, such as increasing custom attributes defined by a plugin i.e. strength, agility ,etc.

@ST-DDT
Copy link
Member

ST-DDT commented May 16, 2015

I guess this is possible using AttributeModifiers:
org.spongepowered.ap.attribute
org.spongepowered.api.data.manipulators.AttributeData
I'm not sure but there seems to be some issue with the API there.
AttributeData extends ListData<AttributeModifier, AttributeData>
provides AttributeModifiers, but there is no way to get them for a single attribute.
Maybe AttributeData should be changed to
extends MappedData<Attribute, AttributeModifier, AttributeData>

@ST-DDT
Copy link
Member

ST-DDT commented May 16, 2015

@AlphaModder @gabizou Can you check whether
AttributeData extends ListData<AttributeModifier, AttributeData>
is right?
IMO it should be:
AttributeData extends MappedData<Attribute, AttributeModifier, AttributeData>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants