Goats do not move correctly after they are tempted during a long jump

Yeah, seems like the physics didn't properly get reset when the goat landed.

From yesterday's Developer Q&A on Discord:

Q: What was the most difficult aspect of coding the goat? A: For sure the Jumping! We first started with a very "naive" approach where we just "transformed" the goat in a "nice" arc. However we realized this caused many issues, so we went back to the drawing board and landed on the idea of giving the goat an initial velocity and then let physics do its part 🙂 However, calculating Projectile Motion while taking air resistance in consideration becomes more complicated, so we ended up turning off air resistance for the mob while it jumps

https://discord.com/channels/302094807046684672/851957982286708766/852581395331416085

So perhaps air resistance isn't turned on again after the goat landed?

I've attempted to reproduce this issue multiple times and have been unsuccessful. With that being said, are there any particular steps that you can provide in order to reproduce this?

I probably should have updated this bug report 2 months ago when I figured it out...
https://www.youtube.com/watch?v=HNKp8Ap13p4

It's just another "holding food" bug that the goat has, so it's related to:
https://bugs.mojang.com/browse/MC-225195
https://bugs.mojang.com/browse/MC-226103

Also relates to:
https://bugs.mojang.com/browse/MC-225963

All these bugs are caused because the Goats & Axolotls are using a new AI system that prioritizes tasks. Food & Lead are highest priority, when they should be lower then things like scare, ram, jump, etc...

Confirmed! Thank you @unknown for the video – it was very helpful in understanding the issue. 🙂

Still in 1.18 pre-1

In 22w05a

Can confirm in 1.18.1.

This ticket closely relates to MC-249145.

Code Analysis - Yarn 22w11a
First, we will start by looking at mobTick() inside of the GoatEntity

net.minecraft.entity.passive.GoatEntity.java

protected void mobTick() { // Removed profiling
	this.getBrain().tick((ServerWorld)this.world, this);
	GoatBrain.updateActivities(this); // This is what's important! Will run every tick
	super.mobTick();
}

So let's first look at how the jump task gets initialized, and the restrictions it must follow

net.minecraft.entity.passive.GoatBrain.java

//This adds the long jump task to the goats AI tasks list, this happens when a goat entity is created
private static void addLongJumpActivities(Brain<GoatEntity> brain) {
	brain.setTaskList( // This method is shown in the code block below
		Activity.LONG_JUMP, // This task is registered as the LONG_JUMP activity
		ImmutableList.of(
			Pair.of(0, new LeapingChargeTask(/* ... */)),
			Pair.of(1, new LongJumpTask<>(/* ... */))
		),
		// This is a set of requirements for the Activity to keep running!
		ImmutableSet.of(
			Pair.of(MemoryModuleType.TEMPTING_PLAYER, MemoryModuleState.VALUE_ABSENT),
			Pair.of(MemoryModuleType.BREED_TARGET, MemoryModuleState.VALUE_ABSENT),
			Pair.of(MemoryModuleType.WALK_TARGET, MemoryModuleState.VALUE_ABSENT),
			Pair.of(MemoryModuleType.LONG_JUMP_COOLING_DOWN, MemoryModuleState.VALUE_ABSENT)
		)
	);
}

// Here is the method that was being called from within Goat Entity. We will check where that leads in a bit
public static void updateActivities(GoatEntity goat) {
	goat.getBrain().resetPossibleActivities(
		ImmutableList.of(Activity.RAM, Activity.LONG_JUMP, Activity.IDLE)
	);
}

net.minecraft.entity.ai.brain.Brain.java

// Some arguments where made unchecked so it's easier to understand and read
public void setTaskList(Activity activity, ImmutableList t, Set requiredMemories) {
	//Just calls method below
	this.setTaskList(activity, t, requiredMemories, Sets.<MemoryModuleType<?>>newHashSet());
}

public void setTaskList(Activity activity, ImmutableList t, Set requiredMemories, Set forgettingMemories) {
	// Here it adds the list of requirements to a variable named: `requiredActivityMemories`
	this.requiredActivityMemories.put(activity, requiredMemories);
	// *removed*
}

// This is what's called every mobTick from Goat Entity with [`RAM`, `LONG_JUMP`, `IDLE`]
public void resetPossibleActivities(List<Activity> activities) {
	for(Activity activity : activities) {
		if (this.canDoActivity(activity)) { // If the activity can be done
			this.resetPossibleActivities(activity); // IDLE would be the only one to pass
			break;
		}
	}
}

// We are going to be looking at Activity.LONG_JUMP since that's the one related to this bug
private boolean canDoActivity(Activity activity) {
	// Long_Jump was added to requiredActivityMemories in the 2nd method of this code block
	if (!this.requiredActivityMemories.containsKey(activity)) return false;
	// This checks if all the required memories are met for the activity
	for(Pair<MemoryModuleType<?>, MemoryModuleState> p : this.requiredActivityMemories.get(activity)) {
		// In the case of LONG_JUMP, TEMPTING_PLAYER is no longer VALUE_ABSENT. So it returns false
		if (!this.isMemoryInState(p.getFirst(), p.getSecond())) return false;
	}
	return true;
}

// This runs with IDLE as show 2 methods above
private void resetPossibleActivities(Activity except) {
	if (!this.possibleActivities.contains(activity)) { // Was a method call, simplified
		this.forgetIrrelevantMemories(except); // Does not apply to this
		this.possibleActivities.clear(); // Removes the active LONG_JUMP
		this.possibleActivities.addAll(this.coreActivities); // Adds CORE
		this.possibleActivities.add(except); // Adds IDLE
	}
}

I'm so sorry if you actually read through these and don't know how to code, I try to make it readable but...

Alright so what's wrong here, why does this happen?

Basically, if the TEMPTING_PLAYER memory module type gets set to a value, the LONG_JUMP activity gets suspended.
Isn't there a check to prevent the TEMPTING_PLAYER from starting while the goat is jumping?
Why YES!

net.minecraft.entity.passive.GoatBrain.java

private static void addIdleActivities(Brain<GoatEntity> brain) {
	brain.setTaskList(
		Activity.IDLE, // Activity IDLE is where the TemptTask runs
		ImmutableList.of(
			Pair.of(0, new TimeLimitedTask<>(/* ... */),
			Pair.of(0, new BreedTask(/* ... */),
			Pair.of(1, new TemptTask(goat -> 1.25F)), // Here it is
			Pair.of(2, new WalkTowardClosestAdultTask<>(/* ... */)),
			Pair.of(3, new RandomTask<>(/* ... */)))
		),
		// As you can see here, the IDLE activity is restricted if LONG_JUMP_MID_JUMP is active
		ImmutableSet.of(
			Pair.of(MemoryModuleType.RAM_TARGET, MemoryModuleState.VALUE_ABSENT), 
			Pair.of(MemoryModuleType.LONG_JUMP_MID_JUMP, MemoryModuleState.VALUE_ABSENT)
		)
	);
}

So as you can see above the IDLE task where tempting a goat runs, it's restricted to make sure it can't run during the Long_jump task. So the question now becomes, why is it still running?

The problem is that LONG_JUMP_MID_JUMP is not set when the LONG_JUMP activity starts, it's set directly after removing the friction from the goat. Over in the LongJumpTask:

net.minecraft.entity.ai.brain.task.LongJumpTask.java

protected void keepRunning(ServerWorld serverWorld, E mobEntity, long l) {
	if (this.lastTarget != null) {
		if (l - this.targetTime >= 40L) { // This is once it's done tracking and decides to jump
			mobEntity.setYaw(mobEntity.bodyYaw);
			mobEntity.setNoDrag(true); // Friction gets removed here
			double d = this.lastTarget.length();
			double e = d + mobEntity.getJumpBoostVelocityModifier();
			mobEntity.setVelocity(this.lastTarget.multiply(e / d));
			mobEntity.getBrain().remember(MemoryModuleType.LONG_JUMP_MID_JUMP, true); // HERE
			serverWorld.playSoundFromEntity(/* ... */);
		}
	} else {
		--this.cooldown;
		this.method_41342(serverWorld, mobEntity, l);
	}
}

So how do we fix this, well honestly it's a bit of a pickle. Although I think you just got the wrong idea, the memory module states are meant to be global values shared throughout multiple tasks. Although the current uses of LONG_JUMP_MID_JUMP make it seem more like a glorified boolean.
So instead LONG_JUMP_MID_JUMP should be renamed to something along the lines of LONG_JUMP_RUNNING and set to TRUE the second that the Long_Jump Activity starts, and instead a protected boolean should be used to keep track of if the goat is in the air or not.
This ensures that other tasks can't override when they are not supposed to.
Or... you have a new argument for setTaskList where you can input activities that the current activity can't run during/override.

Now that the long_jump task is suspended, it can't run since the tempting_player is running. So now the goat is frictionless until you stop tempting it.

Once I make my fix I will add it to this comment, although it's going to be pretty jank since this system is very spread out, and mixins aren't as great as I wish they were. I will also include screenshots of my debugger running and showing the values as proof that this is actually the case.

Edit:
After attempting to make the fix, it seems you never set the Drag back to true, since there is no finishRunning() within the LongJumpTask. Instead, you were relying on the LeapingChargeTask to do that for you, although it finishes instantly.

Very Simple working fix: https://github.com/fxmorin/carpet-fixes/commit/8c2f1889930d4b804d1e1d9387fed93383846559

Works by using the cooldown with VALUE_PRESENT instead of the MID_JUMP with VALUE_ABSENT