More message filters #5605
More message filters #5605
Conversation
wookieejedi
added
enhancement
code/mission/missionmessage.cpp
Outdated
| @@ -403,6 +427,16 @@ void handle_legacy_backup_message(MissionMessage& msg, SCP_string wing_name) { | |||
| strcpy(msg.name, backup); | |||
| } | |||
|
|
|||
| int lookup_mood(SCP_string& name) { | |||
There was a problem hiding this comment.
We should be sticking to making things like this name parameter const as often as we can
There was a problem hiding this comment.
This is somewhere where my C++ is a little lacking. SCP_string const & means "a reference to an SCP_string which we promise not to change" and potentially allows for compiler optimizations, right? What about declaring the function itself const? (To be clear, I'm asking from an educational perspective ^^ )
There was a problem hiding this comment.
Yes, basically.
SCP_string const & gives you a reference to a string you cannot change. The compiler will stop you from any things that would change the string, so not only allows it for more optimization, but also is simply a safeguard for you as the developer to not do things you shouldn't.
Declaring the function const in this context is not possible. You could declare the return type const (const int lookup_mood), but declaring the function const (int lookup_mood(...) const {) is only possible if it's a member function of a class (which it isn't, as a global function), and will basically mean "disallow any modification of member variables", which allows you to call the function on an object of that class which is const itself.
| msg.mood = 0; | ||
| SCP_string buf; | ||
| stuff_string(buf, F_NAME); | ||
| msg.mood = lookup_mood(buf); |
There was a problem hiding this comment.
If a mood isn't found, msg.mood will now be set to -1 instead of 0. Which in turn means that erroneous moods for messages caused the messages to be boosted when the mood was actually 0.
While I doubt that that has been taken advantage off in an existing mod, it should probably at least get a comment here acknowledging that change, in case anyone comes looking in the future.
There was a problem hiding this comment.
Would it? Looking at the code on master, I'm not sure that msg.mood is actually initialized on a missing mood. Isn't that UB?
e: following up on Discord
There was a problem hiding this comment.
True, turns out I was confusing things, and the default-initialization implicitly causing a zero-initialization is only correct for variables with a static lifetime
code/mission/missionmessage.cpp
Outdated
| if (obj->flags[Object::Object_Flags::Should_be_dead]) { | ||
| continue; | ||
| } | ||
| if ((range >= 0) && (vm_vec_dist(&obj->pos, &Objects[sender->objnum].pos) > range)) { |
There was a problem hiding this comment.
>= ?
I mean, I guess an uninitialized range is set to -1, but is there ever a case where range == 0 is sensible?
There was a problem hiding this comment.
Since the sender itself is ignored (if (sender->objnum == i->objnum) { continue; }), the only way a range of 0 could ever be satisfied is with non-colliding objects in exactly the same position. I don't think it matters in practice, but > 0 is probably clearer, so I'm happy to make that change.
JohnAFernandez
removed
the
Waiting for Stable
This PR introduces a few enhancements to the dynamic message system:
All of these features are desired for BTA's upcoming release.