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.
|
Hi guys. I don't want to sound pushy but do we have an ETA or target date for this? If it's going to be a while I'll need to plan around it. Also if I were to use the 'old' set-guard-range Sexp in a mission will I need to go back and change it for the new one once it's ready? Cheers 👍 |
|
Sorry for the delay, my life got super busy suddenly out of nowhere and I hadn't had the chance to look at this. Baring any issues the version I just pushed should be ready to go in. I did have to rip out the scripting support as I simply don't have time right now to learn how to rewrite it for how we now do the sexp. If you have used the old sexp you will have to update but it should be obvious where as it should fail on running the sexp as it wont have enough parameters. |
4 participants
New guard-range sexp which allows different ranges for different guarders. As requested by Iain baker.