New guard-range sexp - #7606
Conversation
New guard-range sexp which allows different ranges for different guarders
TheForce172
added
feature
|
Can this not be done as an expansion of the old sexp instead of adding yet another -new sexp? I know there's convention for that but it's not really a user friendly convention. |
|
Not without changeing the old sexp's behaviour which were not allowed to do as it's been in a stable release. |
|
I suspect I'm the only dev who has used the 'old' set-guard-range Sexp since 26 is so new and I have no issue with adding this to the existing Sexp 🙂 |
|
I did a poll to ask if anyone had used the set-guard-range Sexp and it looks like no one has except myself, and most people didn't even know it was 'a thing', so I think we can safely add to it without upsetting anyone. |
Goober5000
left a comment
Goober5000
left a comment
There was a problem hiding this comment.
Got a review for this which I'll send over Discord.
|
Ok, Is clang having a nervous breakdown today or something? This one makes no sense either? |
The codebase in general prefers EDIT: Oh I see.. it's saying there's a NULL on that line when there's clearly not. Uh. That's neat. |
Goober5000
left a comment
Goober5000
left a comment
There was a problem hiding this comment.
Had some time to look at this more closely now. A few more comments inline. Also:
- The
set-guard-rangedefinition in theOperatorsarray still has 2 minimum arguments. It should have 3. - Since you've made extensive modifications to the SEXP, you could add your name as a co-author.
- The
warp_shipp = NULL;checks are clang checking unrelated lines that weren't included in the diff. They are fromship_render. Annoying, but just two lines to tweak and the warnings should go away.
| case OP_SET_DEBRIS_FIELD: | ||
| case OP_NEBULA_TOGGLE_POOF: | ||
| case OP_NEBULA_FADE_POOF: | ||
| case OP_SET_GUARD_RANGE: |
There was a problem hiding this comment.
This should be removed now that OP_SET_GUARD_RANGE is being modified instead of replaced.
| shipp->max_guard_radius = new_max_guard_radius; | ||
|
|
||
| return ade_set_args(L, "f", shipp->max_guard_radius); | ||
| return ADE_RETURN_NIL; |
There was a problem hiding this comment.
Getting MaxGuardRadius should return a value here, not nil. It will have to build the value by iterating through max_guard_ranges.
| float max_guard_radius; // Optional clamp for guard engagement/resume ranges; <= 0 means unused | ||
|
|
||
| SCP_vector<guard_range_entry> | ||
| max_guard_ranges; // Optional clamp for guard engagement/resume ranges; <= 0 means unused |
There was a problem hiding this comment.
The <= 0 means unused comment is drift from the original numeric implementation. It should be updated
|
|
||
| //************************************************************** | ||
| void set_guard_range_ship(float range, const ship_registry_entry* ship_entry, ship* shipp); | ||
| //************************************************************** |
There was a problem hiding this comment.
this indent ought to be fixed
| if (configured > 0.0f) { | ||
| return configured; | ||
| for (const auto& range_entry : guarder_ship->max_guard_ranges) { | ||
| const int regester = ship_registry_get_index(Ships[guarded_objp->instance].ship_name); |
There was a problem hiding this comment.
Looking at Claude's previous review a bit more closely, it noticed the right symptom but made the wrong recommendation. Since the guard information is only valid for the lifetime of a ship, it is appropriate to use the shipnum index into Ships[] as the stored index. The ship registry index adds complexity and extra processing for no benefit.
Now, doing this will risk leaving a stale guarded shipnum when the ship is destroyed, so the solution to that is to clean it up in ai_ship_destroy. You can check for stale guard thresholds in the "wipe out this ship as a target" loop.
4 participants
New guard-range sexp which allows different ranges for different guarders. As requested by Iain baker.