Add entity API

[lonevox]

PLEASE READ THE GUIDELINES BEFORE MAKING A CONTRIBUTION

• Please check if the PR fulfills these requirements

• The commit message are well described
• Docs have been added / updated (for features or maybe bugs which were noted). If not, please update the needed documentation here. This is not mandatory
• All changes have fully been tested

• What kind of change does this PR introduce? (Bug fix, feature, ...)
  Feature.

• What is the current behavior? (You can also link to an open issue here)
  Calling EnvironmentDetectorPeripheral.scanEntities() gives only a small amount of information on each entity. I felt like this could be used to a greater potential.

• What is the new behavior (if this is a feature change)?
  Instead of getting a table with a small amount of data when calling EnvironmentDetectorPeripheral.scanEntities(), UUIDs for entities are now the only thing that is returned. These can be used with the new entity API. For example, entity.getNBT(uuid) returns the NBT data of the entity with the given UUID.
  The entity API itself can be disabled in config, as well as the individual functions within it that could be deemed exploitative on servers.
  A PlayerDetectorPeripheral.getPlayerUUID(username) Lua function has also been added so that you can make use of the entity API when using a PlayerDetectorPeripheral.
  I have also expanded the amount of entity information you get when calling LuaConverter.completeEntityToLua(entity). This function can be called via the entity API with entity.getData(uuid).

• Does this PR introduce a breaking change? (What changes might users need to make in their scripts due to this PR?)
  Yes, any calls to EnvironmentDetectorPeripheral.scanEntities() will need to use the entity API to get the data that it used to give you.
  Calls to AutomataEntityHandPlugin.searchAnimals() now have position information in a table with the key relativePos. This was a side-effect of cleaning up LuaConverter.completeEntityToLua(entity) to not require an ItemStack to be passed to it.

• Other information:
  I can make a PR for the docs if this PR is to be merged.

[lonevox]

Here's a script that I've been testing with: https://pastebin.com/uCH39VEm

[SirEndii]

Before I start reviewing

Since this adds a wider palette of features and due to the fact that this includes a breaking change, could you change the target branch to dev/0.8 so this features will be implemented to the next major version?

[lonevox]

Done

[SirEndii]

Greetings

The build seems to fail, I am not sure if the issues came with the change of the target branch, but they are there
Could you fix them? I would also recommend running gradle check afterward to check for checkstyle rule violations.

You can either open the build log from teamcity by clicking on "Details" or you just could try to build it locally

[lonevox]

My bad, it builds now

[SirEndii]

You still didn't fix the checkstyle violations
Either check the teamcity build log or run gradle check locally

[SirEndii]

Foremost, thanks for the PR and I really like your idea
I also discussed that internally with some collaborators, and we agree that switching to an LuaAPI based entity api would be a nice addition to AP

The changes look good, but I didn't inspect them too deeply. I will take some time this week to review your changes and test them a bit for me

[tehgreatdoge]

Would it have made more sense to implement this with peripheral methods instead of polluting the global namespace? You could return a userdata from the scan method, return all the information by default, or just add another method to the peripheral for looking up data by uuid

[lonevox]

Would it have made more sense to implement this with peripheral methods instead of polluting the global namespace?

Originally I started with adding a getNBT(UUID) function to EnvironmentDetectorPeripheral, since I didnt want to put the huge NBT information in the small table returned by scanEntities, but if you think about it it makes no sense to have a getNBT function in EnvironmentDetectorPeripheral. It was either have a massive table returned or separate this into an Entity API which can have more fine-grained functions, so I opted for the later. It also isn't clear which functions in different peripherals return what entity data, but if you get a UUID from EnvironmentDetectorPeripheral.scanEntities and a UUID from PlayerDetectorPeripheral.getPlayerUUID, you know that Entity.getData(UUID) will return the same type of information for both entities.

You could return a userdata from the scan method

It was already the case before that some functions returned different entity data than others for no real reason. scanEntities only gave you like 10 fields per entity so it wasn't very useful, whereas searchAnimals gave you a lot of information, but still didn't include NBT data. It is easier to maintain and more consistent if this is all in one place.

return all the information by default

You could return all data, but there is a lot of information available now. It feels almost wrong to lump almost all information on the entity into one huge table. This would be NBT, almost all runtime data, and all persistent data. If you just need to get the positions of entities then this is all useless information.

or just add another method to the peripheral for looking up data by uuid

Something like getEntityData(UUID) wouldn't make sense in any one peripheral

[lonevox]

After looking at some of my changes again though, I think it would make sense for AutomataEntityHandPlugin.searchAnimals and AutomataEntityTransferPlugin.getCapturedAnimal to both return UUIDs instead of the result of completeEntityToLuaWithShearable to be consistent with my other changes

[SirEndii]

Would it have made more sense to implement this with peripheral methods instead of polluting the global namespace? You could return a userdata from the scan method, return all the information by default, or just add another method to the peripheral for looking up data by uuid

I am currently discussing that a bit with some of the CC community in the CC discord. I know that additions to the global API are not welcome in the community. AP already does unconventional things that I want to eliminate in 0.8, and I'd hate to add more.

[lonevox]

I am currently discussing that a bit with some of the CC community in the CC discord. I know that additions to the global API are not welcome in the community. AP already does unconventional things that I want to eliminate in 0.8, and I'd hate to add more.

Damn thats unfortunate, I wasn't aware of that. It's an issue of usability and code quality vs global pollution then. It's a shame that an entity API isn't available in CC itself.

I guess a rough compromise could be standardizing the entity data returned in different parts of this mod. So pretty much chucking everything into one huge table, but making sure that happens consistently in every peripheral function where an entity is retrieved. It would be worth documenting this standard entity table somewhere.

Another alternative is turning the Player Detector peripheral into an Entity Detector peripheral. All functionality of the entity API could be put in the Entity Detector. scanEntities could be removed from the Environment Detector and put into the Entity Detector.

[SirEndii]

Another alternative is turning the Player Detector peripheral into an Entity Detector peripheral. All functionality of the entity API could be put in the Entity Detector. scanEntities could be removed from the Environment Detector and put into the Entity Detector.

I support that idea
Adding all the entity related features to one block would be the ideal compromise imo

you could also participate in the small discussion in the #modmaking chat on the cc discord

[SirEndii]

One idea I like is to still add all the entity API functions to the environmental detector but to separate them a bit, put them into a table which you can retrieve using getEntityAPI.
This would be simpler than "adding the API to the global environment, but only when the peripheral is attached" imo

So maybe you want to go with that?

I will also convert this to a draft until we agree on a method

[lonevox]

One idea I like is to still add all the entity API functions to the environmental detector but to separate them a bit, put them into a table which you can retrieve using getEntityAPI. This would be simpler than "adding the API to the global environment, but only when the peripheral is attached" imo

So maybe you want to go with that?

I will also convert this to a draft until we agree on a method

For the idea of turning the Player Detector into an Entity Detector:
Player Detector has four player-specific things: the playerClick, playerJoin, playerLeave events, and the getOnlinePlayers function. Everything else can be made to work for any entity (1 event and 10 functions). The getPlayers... functions currently return the names of players, but they could become getEntities... functions and return entity UUIDs instead. The functions migrated from the EntityAPI like getName(UUID) or getNBT(UUID) would have checks within them to see if the entity with the UUID is still within the range of the Entity Detector so that these can't be abused to get information about entities anywhere.

I think a getEntityAPI on the Environment Detector is a possibility, although you'd then have almost duplicate functions across peripherals (such as EnvironmentDetector.EntityAPI.getPos and PlayerDetector.getPlayerPos). An Entity Detector could replace all functions that get entity data in all other peripherals.

The biggest consideration with the potential Entity Detector is if its okay for it to have the 4 player-specific things. These 4 things cant really be removed, so its just if it makes sense or not. If it doesn't make sense, then it might not make sense to turn the Player Detector into an Entity Detector. I'm not sure, I'd like your opinion.

[SirEndii]

Merging the two peripherals is an interesting idea, but I don't really like it. Merging them would create one peripheral with maybe too many functions imo which wouldn't keep it simple

Developing the Player Detector and the Environment Detector separately would be my way to go. I would simply accept the duplicates, as there are apparently only 2 functions (getPlayerPos and getOnlinePlayers)

Keeping the player related functions (isPlayerInX... getPlayersInX...) and events to the player detector would make it simpler for the user imo to just have player related functions in one dedicated block while there would be the more advanced entity api in the Environment Detector

[lonevox]

Sweet. I'm going away for a week but I'll work on that when I get back

[SirEndii]

Greetings @lonevox
Do you have any plans on continuing this?

[lonevox]

Greetings @lonevox Do you have any plans on continuing this?

I took quite a hiatus from making my pack which is why I completely disappeared, sorry about that. I'll try getting the PR ready with the discussed changes within the next few days.

[lonevox]

@SirEndii I'm having trouble exposing the EntityAPI class as a table through the EnvironmentDetectorPeripheral like you suggested. Here's what I assume would work, but isn't:

@LuaFunction(mainThread = true)
public final EntityAPI getEntityAPI() {
    return new EntityAPI();
}

It successfully returns a table, but the table is empty, even though there are 6 LuaFunctions in EntityAPI. Any ideas?

[SirEndii]

Can you check the log?
CC throws some debug messages when it's unable to parse functions

[zyxkad]

Thanks for your contribution! ❤️
Everything looks good now. However, I'll still do more client test before merging it, just to be responsible ...
I may finish testing this today, or tomorrow.

Still don't know if query with UUID is a good move, because this change force users do more main thread invokes, it increased the response time, and may decrease the ease of use.

[lonevox]

It does decrease ease of use in that you need to manage UUIDs in Lua, but it allows for the EnvironmentDetectorPeripheral, PlayerDetectorPeripheral, and AutomataEntityHandPlugin to use a shared API for getting entity info instead of having duplicate functions.

[zyxkad]

Ok, so when an entity is not exists any more, the entity API will throw a java error, we should not expect that, instead we should return nil, "ENTITY_NOT_EXISTS" or some else message

[zyxkad]

Hm and with the entity API, you can get any entities data once you know their UUID, no matter where they are. That probably is a little bit too powerful, may be abused, and not what we expect.

I'd ask @SirEndii's opinion for that.

---

Currently two solutions I can think of:

1. Check the entities position & range before returns the data.
2. Snapshot the entities' status when invoking scanEntity or such method, search in the snapshot when invoking getData.

However both of them will make EntityAPI depends on the peripheral's status again

[lonevox]

I'm doing slightly weird validation by using Pair, but it is better than before. It would be nice if Java had an Either type or Rusts struct-like enums, but we make do.

[SirEndii]

I'd ask @SirEndii's opinion for that.

I would prefer your option one, seems to be the simplest, and it would follow the same logic as with other functions and peripherals

[lonevox]

I'm currently opting for having EntityAPI be constructed by EnvironmentDetectorPeripheral::getEntityAPI like so (it used to return a static EntityAPI instance):

@LuaFunction
public final EntityAPI getEntityAPI() {
    return new EntityAPI(getPeripheralOwner());
}

This means that within the EntityAPI it can check the distance from the entity to the peripheral whenever a function is called. However, I'm unsure of what range the EntityAPI should have, and how to handle range. It could just be a flat max distance value such as 5 blocks, or it could be a SphereOperation. If the right way to go is a SphereOperation, then here's some options I'm considering:

1. I could add a new SphereOperation value for each EntityAPI function (as EntityAPI::getPos should probably be cheaper than EntityAPI::getNBT based on how much less data is returned) but this would pollute SphereOperation, as it currently has 2 values and I would be adding 6 more.

2. I could add just one SphereOperation value called GET_ENTITY_DATA and have the free radius the same as the cost radius so it is a free operation. The idea behind it being free is the player already needed to run EnvironmentDetectorPeripheral::scanEntities to get the entity UUIDs, however this might as well be free because you can now use those UUIDs to call EntityAPI functions for that entity forever for no extra cost.

3. A compromise is to add 2 SphereOperation values:

   • GET_SMALL_ENTITY_DATA (for getName, getPos, getBoundingBox, and getPersistentData).
   • GET_LARGE_ENTITY_DATA (for getNBT and getData).

Or SphereOperation might not be the way to do this at all. I'd like to know what you guys think.

There is also a whole other issue of the fact that a malicious player could get their hands on a victims EntityAPI instance (since they're instanced by EnvironmentDetectorPeripheral now) and call expensive functions on it from anywhere. This lends itself to unfortunately ditching the EntityAPI and moving the functions to EnvironmentDetectorPeripheral, which is becoming a more attractive option.

[SirEndii]

Sorry for not answering, I completely forgot this PR

I would prefer the use of SphereOperation, your second option sounds more reasonable to me

There is also a whole other issue of the fact that a malicious player could get their hands on a victims EntityAPI instance (since they're instanced by EnvironmentDetectorPeripheral now)

If the function creates a new Entity API for every getEntityAPI() function call, wouldn't a user need to save the entity api inside a local field so the cooldowns and other stuff are stored? If a user always creates a new EntityAPI instance via the function, wouldn't they also create new cooldowns/reset them,

Maybe you're right, and the best way is to move all the functions into the EnvDetector itself. I at least would now prefer that

[SirEndii]

Greetings @lonevox
I would just like to know how things are going for you in terms of the time you have and what else you are planning for the PR

[zyxkad]

I just have an idea that you should not try to use UUID to index entity data at all, you can just returns a table include a method that takes no argument for extra information, such as:

{
  id = x,
  uuid = xx,
  name = xxx,
  type = xxxx,
  getAdvancedInfo = function() return <wrappedJavaMap> end,
  getNBT = function() return <convertedNBTData> end,
}