enhance the add-goal and remove-goal sexps #6453
Conversation
Goober5000
added
enhancement
1. Allow the goal priority to be an evaluated number, not just a literal number. This allows the goal priority to be obtained from operators or calculations. 2. Since the priority can now be evaluated, it can be NaN. In that case, default to the max priority. 3. Consolidate `CDR(CDR(...))` to `CDDR()` in the aigoal implementation 4. Clarify that `remove-goal` removes the first goal of a given type and priority, not necessarily the first fully matching goal. 5. Enhance `remove-goal` to remove all goals matching the constraints, not just the first goal. 6. Allow `remove-goal` to disregard priority when checking for matching goals.
Goober5000
force-pushed
the
add_remove_goal_sexp_updates
branch
from
4ec07ee to
09552bb
Compare
code/ai/aigoals.cpp
Outdated
| @@ -1150,8 +1151,8 @@ void ai_add_goal_sub_sexp( int sexp, int type, ai_info *aip, ai_goal *aigp, cons | |||
| } | |||
| } | |||
|
|
|||
| if ( aigp->priority > MAX_GOAL_PRIORITY ) { | |||
| nprintf (("AI", "bashing sexpression priority of goal %s from %d to %d.\n", CTEXT(node), aigp->priority, MAX_GOAL_PRIORITY)); | |||
| if ( aigp->priority > MAX_GOAL_PRIORITY || priority_is_nan || priority_is_nan_forever ) { | |||
There was a problem hiding this comment.
Assigning a goal with a priority higher than 90 can interfere with the player's ability to issue orders. This is rarely, but not never, done intentionally. With this change, it's possible for it to happen unintentionally, which seems like a footgun in the making. How'd you feel about having it issue a debug warning and cancel the order?
There was a problem hiding this comment.
(Note to self, not particularly related to this PR: check and document how max and min handle NaN.)
There was a problem hiding this comment.
Good points. I'll do that.
(Aside: when eval_num encounters SEXP_NAN, it returns 0 and sets the flags, so aigp->priority would be 0 here.)
| @@ -1245,115 +1247,145 @@ int ai_remove_goal_sexp_sub( int sexp, ai_goal* aigp ) | |||
| /* The operator to use */ | |||
| int op = get_operator_const( node ); | |||
|
|
|||
| // since this logic is common to all goals removed by the remove-goal sexp | |||
| auto eval_priority_et_seq = [sexp, &remove_more](int n, int priority_if_no_n = -1)->int | |||
There was a problem hiding this comment.
I'm familiar with the abbreviation "et seq", but it's not clear to me what it means in this context.
There was a problem hiding this comment.
It's an abbreviated "and the following nodes", i.e. evaluate the priority and the nodes that come after the goal operator: the remove-all flag and the check-priority flag. I imagine that a future enhancement would add another boolean flag for checking the goal target, since right now remove-goal does not distinguish between the same goal with different targets.
| */ | ||
| int ai_remove_goal_sexp_sub( int sexp, ai_goal* aigp ) | ||
| int ai_remove_goal_sexp_sub( int sexp, ai_goal* aigp, bool &remove_more ) | ||
| { |
There was a problem hiding this comment.
Perhaps I'm missing something, but I'm not sure what the advantage is of making the caller responsible for looping. bool remove_all = false would remove that requirement and maintain API compatibility. (That said, this function is almost a misplaced implementation detail, so if it makes the implementation easier or clearer, that's probably fine.)
There was a problem hiding this comment.
It's definitely implementation; I spent a bit of time puzzling out the design before settling on the parameter. Turning ai_remove_goal_sexp_sub() into a loop would be inelegant, but the real kicker is that any given loop iteration might need to deactivate the active goal, and this requires information that only the caller has.
|
Any further thoughts? I plan to merge this in the next day or two. |
CDR(CDR(...))toCDDR()in the aigoal implementationremove-goalremoves the first goal of a given type and priority, not necessarily the first fully matching goal.remove-goalto remove all goals matching the constraints, not just the first goal.remove-goalto disregard priority when checking for matching goals.