Minecraftforge: Fake player triggers monster hunter advancement

[P3pp3rF1y]

While testing my mod port for 1.12 I noticed that fake player triggers monster hunter advancement when killing a mob.

This is because awardKillScore isn't overriden in FakePlayer and thus triggers the advancement as if it was for a regular player.

I have checked the class for all other advancements triggered in there and these are all the methods that have at least one in them:
onInsideBlock
onUpdateEntity
trySleep
sendSlotContents
onNewPotionEffect
onChangedPotionEffect
onFinishedPotionEffect

I don't believe that mods should have to override these in their implementations of fake player just to get rid of the advancement triggers, but before I go ahead and submit PR I would like to check on what the opinion is of the correct change.
Just override all of these with empty method bodies? (with exception of updateEntity just calling onUpdate).
Override fewer of these?
Or do something else here?
I don't see any code in these that would be required for fake players (again with the exception of onUpdate call in updateEntity), but I may very well be missing something.

[williewillus]

Simpler solution but needs 1 vanilla patch (vs many overrides in forge):
Just add a single instanceof check to the top of PlayerAdvancements.grantCriterion, which will prevent fakeplayer from ever getting advancements, even if they are triggered.

[P3pp3rF1y]

Yes I guess that makes the easiest in terms of lines of code changed, but seeing all the real player specific code in these methods I would anyway override those in my implementation of fake player to get rid of unnecessary calls. At that point the instanceof check in forge wouldn't do anything for me.
So my question is really, shouldn't the overrides be done in the forge's FakePlayer implementation?

[mezz]

I think if we can avoid patches we should.
There are a lot of FakePlayer overrides but they're still simple and not invasive at all.

[bs2609]

What about advancements that aren't triggered from player methods?

[mezz]

If you can point out an example where we can't fix it with the FakePlayer overrides, then yeah we need to do it another way.

[bs2609]

Was thinking of things like taming/breeding animals, also any custom advancement triggers added by mods.

[DBotThePony]

[tyler489]

Lol came here to report this https://gyazo.com/e988687dbd2b2445905f54d20c0ef3fb Evilcraft spikes register as [Minecraft] Fake player

[Xalcon]

i'd like to keep the ability to grant advancements to fakeplayers. Advancements arent just achievements, they also contain rewards like recipes or items. Being able to gate certain things behind advancements while still supporting progression for fakeplayers would be nice.

Cant we just remove the announcement itself?

[P3pp3rF1y]

So, i have looked through the code and there's certainly lots of CriteriaTriggers outside of EntityPlayerMP so to stop spam we definitely need a patch that will check for instanceof FakePlayer as @williewillus suggested.
Now I would just add it before the trigger to not even trigger any advancements, but sounds like there may be use cases where we would want to just stop chat spam (in which case we insert the check in PlayerAdvancements.grantCriterion).
@Xalcon you have any examples of where this would be valid for FakePlayer? You also need to keep in mind that advancement progress data usually doesn't get saved for FakePlayers (at least I don't do it and haven't seen any fake player implementation that would do it) which means if there are awards for something that fake players can achieve simple reload of server of world save would clear these and allow the fake player to achieve those again.

[Xalcon]

@Xalcon you have any examples of where this would be valid for FakePlayer? You also need to keep in mind that advancement progress data usually doesn't get saved for FakePlayers (at least I don't do it and haven't seen any fake player implementation that would do it) which means if there are awards for something that fake players can achieve simple reload of server of world save would clear these and allow the fake player to achieve those again.

I wasnt aware fakeplayers dont get saved. I'm currently working on a mod that will use advancements to gate certain features like the ability to craft certain recipes or interact with certain blocks. To still allow fakeplayers to somewhat work with those restrictions, i was thinking i could just check for earned advancements. IIRC, thaumcraft did something similar with its knowledge system. You were able to teach a fakeplayer the research from another player. But I guess this was done differently than I expected.

Dont mind me then, I'll find a way to do my things :)