form-on-wing and stay-still updates

[Goober5000]

Various goal enhancements requested by FotG. The original ai-form-on-wing and ai-stay-still goals automatically cleared all other goals assigned to the same ship or wing, which is not always desired. This adds several flags to goals and ai_profiles to provide more fine-grained control over these behaviors.

Also removes AI_GOAL_PLACEHOLDER_1 since it is not needed; goals higher than AI_GOAL_NONE do not need to be a specific value.

Supersedes #6520. In draft pending testing.

[wookieejedi]

Testing all works as expected with both new behavior and current existing behavior if flags are off, thanks for all the work on this!

[jg18]

A few thoughts from a first pass, but I need to look more closely.

[jg18]

Let's update this to "2 to 3 arguments".

[jg18]

Wondering why what would normally be a breaking enum change is OK? I take it this enum isn't serialized anywhere?

[Goober5000]

It isn't, but as it turned out, I ended up using this placeholder spot for another enum from another PR. I'll rebase this PR and fix the conflicts.

[jg18]

Still studying, but this is what I have so far.

There was a "must fix" in the previous comments about updating the SEXP help text.

[jg18]

Since this code fragment appears again below, I'd highly encourage creating an internal/static helper function for readability/maintainability.

Suggested function signature:

void ai_goal_set_clear_goals_flag_if_needed(ai_goal *aigp, ai_goal_mode mode)

You might want to create a helper function for the check for Do_not_set_override_when_assigning_form_on_wing as well, since both places check it, but up to you.

[jg18]

I think there's now a bug below when setting/updating oldest_index. We first need to check that goals[gindex].ai_mode != AI_GOAL_NONE.

Creating a helper function might be handy to use both here and above, but up to you:

bool ai_goal_is_goal_slot_empty(const ai_goal *goals, int gIndex)

[Goober5000]

That's probably rare in practice, and shouldn't matter since if the active goal is NONE, all the other goals would also be NONE and it would just pick the next index. But fair point.

[jg18]

The name first_empty_index is clearer IMO.

[jg18]

Why are we still dereferencing current_goal below?

[Goober5000]

(addressed elsewhere)

[jg18]

This should probably be const auto, or even better, just spell out the type: const char *.

[Goober5000]

(obsolete)

[jg18]

This should probably be const auto or (better IMHO) const auto *.

[Goober5000]

It's already const by virtue of the return value of ship_registry_get

[jg18]

After we check for Clear_all_goals_first below, we shouldn't refer to current_goal again, right?

So there's more data from current_goal we need to save at this point, right?

Although I'm a bit confused. Since we were clearing goals before, why do we need to backup data from current_goal? Why wasn't this necessary before?

[jg18]

On 2nd thought, instead of saving individual fields from *current_goal, here's what I suggest:

1. Keep an extra ai_goal instance in memory called, say, current_goal_backup.
2. Right before calling ai_clear_ship_goals() below, first copy the contents of *current_goal to current_goal_backup, and then set current_goal to be &current_goal_backup.

That way, current_goal will always be a valid pointer.

Will need to confirm that there aren't unintended consequences from this change.

[Goober5000]

This wasn't necessary before because the implementations of the relevant goals handled the clearing themselves. (In the case of form-on-wing it had a bug analogous to use-after-free, but it worked anyhow). Since the clearing is now generalized, saving the data is now necessary.

Your idea of backing up the current goal is a really good one. I'll implement it with a slight tweak.

[jg18]

@Goober5000 @wookieejedi I looked into whether multi is OK, and I think it is.

When the player sends orders like "Form on my wing" in multi, the result is a call to send_player_order_packet() to notify the server.

The server handles the order in process_player_order_packet(), in which the server calls either hud_squadmsg_send_ship_command() for an order the player sent to a ship or hud_squadmsg_send_wing_command() for an order the player sent to a wing. These are the same functions used to handle the order in single player. So AFAICT the server should handle the new AI flags correctly.

[jg18]

@Goober5000 Found two bugs in ai_remove_goal_sexp_sub() for case OP_AI_FORM_ON_WING:

1. The 1st argument to eval_priority_et_seq() should be the node corresponding to the priority argument and not hardcoded to -1.
2. The default priority should be The_mission.ai_profile->default_form_on_wing_priority and not hardcoded to 99.

[jg18]

@Goober5000 @wookieejedi I'm confused on how this PR meshes with existing priority code for form-on-wing in ai_add_goal_sub_player() (aigoals.cpp:781-783)

	// Goober5000 - same with form-on-wing, since it's a type of staying near
	else if ( mode == AI_GOAL_FORM_ON_WING )
		aigp->priority = PLAYER_PRIORITY_SUPPORT_LOW;

Shouldn't we be setting aigp->priority to The_mission.ai_profile->default_form_on_wing_priority instead?

This would be quite a behavior change WRT priority boost, though, since PLAYER_PRIORITY_SUPPORT_LOW is just 10.

One idea: set the priority to The_mission.ai_profile->default_form_on_wing_priority if the goal was added via player-issued order (see this comment), otherwise fall back to PLAYER_PRIORITY_SUPPORT_LOW.

As an aside, I wonder if the above code snippet explains why it always felt like issuing a "Form on my wing" order had no effect back when I used to play.

[jg18]

Semi-related: strange that the switch block in error_check_initial_orders() in fredview.cpp doesn't include a case for AI_GOAL_FORM_ON_WING. I wonder why. Ditto for CFred_mission_save::save_ai_goals() in missionsave.cpp and ShipGoalsDlg::initialize() (and other functions) in shipgoalsdlg.cpp. I guess it's because FSO automatically sets "form on wing" as the default AI goal if a ship has none.

[jg18]

The giveOrder Lua function doesn't support using The_mission.ai_profile->default_form_on_wing_priority with form-on-wing orders. Should it?

[jg18]

Some requests/suggestions around setting priority for these AI goals via SEXP.

[jg18]

If we make priority the 2nd argument for ai-form-on-wing, then there's no way for the FREDder to customize the boolean flags (remaining optional arguments) and simultaneously leverage the default form-on-wing priority.

Therefore, consider making priority the 5th argument instead.

[jg18]

@wookieejedi said it's more important not to break existing missions, so scratch this idea.

Presumably, if being able to use the default priority while specifying other flags were important, the SEXP would have been designed with the priority as the last arg.

[Goober5000]

Yeah, most of the optional sexp flags were added after the priority arguments already existed.

[Goober5000]

error_check_initial_orders

Added a case for this in FRED and qtFRED.

save_ai_goals

Added save code

(functions in shipgoalsdlg.cpp)

Done, plus the equivalent in qtFRED

The giveOrder Lua function doesn't support using The_mission.ai_profile->default_form_on_wing_priority with form-on-wing orders. Should it?

Hmm, it probably should.

[Goober5000]

@Goober5000 @wookieejedi I'm confused on how this PR meshes with existing priority code for form-on-wing in ai_add_goal_sub_player() (aigoals.cpp:781-783)

	// Goober5000 - same with form-on-wing, since it's a type of staying near
	else if ( mode == AI_GOAL_FORM_ON_WING )
		aigp->priority = PLAYER_PRIORITY_SUPPORT_LOW;

Shouldn't we be setting aigp->priority to The_mission.ai_profile->default_form_on_wing_priority instead?

Not necessarily, because that is intended for orders given specifically via sexp. In the messaging system, orders always had different priorities:

This would be quite a behavior change WRT priority boost, though, since PLAYER_PRIORITY_SUPPORT_LOW is just 10.

That itself is a behavior change from retail, as those lines were added in 09d116f. I think the best thing to do is remove those lines to restore the original squad-commanded behavior. Presumably with the fixes to the codebase over the years and in this PR, it will cause form-on-wing to work properly, but we'll test it.

As an aside, I wonder if the above code snippet explains why it always felt like issuing a "Form on my wing" order had no effect back when I used to play.

Those lines were intended to fix that exact bug, but if the order had no effect for you, they probably didn't do their job.

[jg18]

Good improvements. Still studying, but a few questions so far.

[jg18]

Does this comment still make sense? Did it make sense even before this change?

[Goober5000]

Well, yes. For example, issuing an attack order causes any previously issued ignore orders to be removed.

[jg18]

To clarify, I was referring to the "next statement" part of the comment.

Presumably, the list of relevant goal types was in the if condition at one point, and then the list was moved to a helper function, so the relevant goal types no longer appear here.

Anyway, it's not important, as long as coders know what it means.

[Goober5000]

Oh, I see what you mean. I'll tweak the comment.

[wookieejedi]

Alrighty, at long last the PR is ready for review :D

[jg18]

We've agreed that I'll pre-approve the updated version now and then re-review in depth during 25.0 RC phase. The RC cycle shouldn't be blocked by a re-review of this PR.