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

Add Mob Goal API #2619

Open
wants to merge 6 commits into
base: master
from
Open

Conversation

@MiniDigger
Copy link
Contributor

MiniDigger commented Oct 6, 2019

Implements #2617

This first MVP-PR doesn't cover handling vanilla goals as good as I would want to: you can't create vanilla goals.
You can list and remove them tho (in addition to implementing your own goals), so this is already a step in the right direction.

The VanilllaGoal class has a list of constants with namespaced keys that vanilla uses. Its generated with the unit test (that also checks its completion)
This allows you to do stuff like this:

Zombie zombie = (Zombie) me.getWorld().spawnEntity(me.getLocation(), EntityType.ZOMBIE);
zombie.getGoalSelector().removeGoal(VanillaGoal.LOOK_AT_PLAYER);
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 6, 2019

(am gonna fix the tests tomorrow/the next time I do changes)

@aikar

This comment has been minimized.

Copy link
Member

aikar commented Oct 6, 2019

the main thing I want to chime in is please avoid using too much bukkit internal naming.

Goals is a valid name for the API, but I feel like 'Selector' does not fit. selection is an internal implementation detail.
A goals API could cover more than the selection of a goal.

I think just dropping the 'Selector' and making it entity.getGoals().add/removeGoal/getGoals is a clean and unlimiting API.

@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 6, 2019

fair point, I also find the bukkit naming hella confusing, thats why I didn't bring the Pathfinder naming into this. I am fine with dropping the Selector in the getters to, but what would I call the GoalSelector class then?
I just looked at MCP and mojang mappings, they both actually call it GoalSelector too (and the fields goalSelector and targetSelector)

@aikar

This comment has been minimized.

Copy link
Member

aikar commented Oct 6, 2019

Implementation wise I can understand the use of the word Selector. but API's should not be tightly coupled to implementation design.
for pathfinder, i went with a simple
./src/main/java/com/destroystokyo/paper/entity/Pathfinder.java

this could be 'Goals', EntityGoals (probably best) or even GoalManager. though I like the cleanness of skipping the word manager.

so maybe entity.getEntityGoals()?

@aikar

This comment has been minimized.

Copy link
Member

aikar commented Oct 6, 2019

or for correctness, since this only applies to mobs, .getMobGoals() and interface MobGoals

@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 6, 2019

ok, so getMobGoals and getMobTargets, which both return MobGoals?
This is already kinda inconsistent again :/

to make that consistent we could have getGoalManager and getTargetManager and change the type to AIManager? not really happy about those suffixes, but getTargets might be confusing since there is already a getTarget method.

@aikar

This comment has been minimized.

Copy link
Member

aikar commented Oct 6, 2019

Could have a single interface for managing goals as MobGoals.
goals consist of 2 types of goals, a targeting goal, and behavioral goals

I would say go with 1 MobGoals interface that has .addTargetGoal and .addBehaviorGoal
This lines up with how the implementation is, but doesn't lock us to that.

@yannicklamprecht

This comment has been minimized.

Copy link

yannicklamprecht commented Oct 6, 2019

I remember that the differentiation of behaviour and target goals got dropped in 1.14.x. Am I right?

@BillyGalbreath

This comment has been minimized.

Copy link
Contributor

BillyGalbreath commented Oct 6, 2019

I remember that the differentiation of behaviour and target goals got dropped in 1.14.x. Am I right?

Nope. Still very much a part of the system, even in 1.15 ;)

@aikar

This comment has been minimized.

Copy link
Member

aikar commented Oct 6, 2019

Future proof way to handle it even if so, is to make a GoalType interface/enum (iface if want to support custom types?) and make target/behavior values, and in future if they merge, then the 2 values can be treated equally.

then .addGoal(type, key, instance)
.getGoals(type)
.getGoal(type, key)

@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 12, 2019

Changed the API to what aikar suggested in his last comment

@MiniDigger MiniDigger requested a review from aikar Oct 12, 2019
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 17, 2019

Since aikar seems busy: anybody else has feedback on this?

@yannicklamprecht

This comment has been minimized.

Copy link

yannicklamprecht commented Oct 17, 2019

I'm fine with that but I'm prejudiced.

@stefvanschie

This comment has been minimized.

Copy link
Contributor

stefvanschie commented Oct 18, 2019

I mentioned some things in the Discord guild yesterday after you said you wanted feedback, but I think it got lost in the stream of messages, so I'll post it here as well (if you already read it, then feel free to ignore this).

In the javadoc of tick you have: "Called each this the goal is activated", should probably be "Called each tick the goal is active" or "Called each time the goal is activated" (not really sure which one you want, I'd assume the first?). Second of all in the javadoc for MobGoals: "Represents a part of the "brain" of an entity.". This might be confusing, since all mobs have a Brain NBT Tag and - please correct me if I'm wrong - these goals do not interact with that. I know this is pretty nitpicky and most people will probably have no problem udnerstanding this, so feel free to just ingore this, but I thought I'd mention it anyway. (Also, might want to change entity to mob in there, since it's for MobGoals, not EntityGoals?)

@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Oct 18, 2019

@stefvanschie the brain part refers to memories, not "thoughts" like theses target and behavior goals. Not really sure if thats more confusing or if having the brain part in the javadoc actually clarifies stuff, I personally find it good, it helps me to figure out what the class does.
Addressed the other two things.

@zachbr zachbr force-pushed the PaperMC:master branch from e38aa24 to 86daffa Dec 10, 2019
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Dec 30, 2019

@MiniDigger MiniDigger force-pushed the MiniDigger:feature/mob-ai-goals branch from fe10c7e to bf387f6 Jan 3, 2020
@MiniDigger MiniDigger force-pushed the MiniDigger:feature/mob-ai-goals branch from bf387f6 to 06593b1 Jan 3, 2020
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Jan 3, 2020

Copy link
Contributor

Phoenix616 left a comment

Added some input regarding the API. I don't really know much of the internal stuff regarding pathfinders so I can't really give any input on that.

Also it looks like there were some internal changes in 1.14 that might be worth exposing (e.g. PathfinderGoal.Type).

Spigot-API-Patches/0189-Add-Mob-Goal-API.patch Outdated Show resolved Hide resolved
Spigot-API-Patches/0189-Add-Mob-Goal-API.patch Outdated Show resolved Hide resolved
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Jan 17, 2020

exposed the sub type and converted type into an enum

…mes for unmapped classes
@MiniDigger

This comment has been minimized.

Copy link
Contributor Author

MiniDigger commented Jan 20, 2020

rebased and addressed the reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.