Well, I am going to pick it apart line by line:
- Why was the style of the comments for HugGaugeMessages::render changed from doxygen style to something else. Why did HugGaugeTalkingHead lose it documentation?
- All directories and code files should have lowercase names.
- Style changes should be a separate patch done under its own merit.
- You are adding an empty file? (RoundRobinSelection)
- Please use doxygen style comments.
- Please be careful of unnecessary changes to the project files and if you are changing compilation options, it should also be a separate patch under its own merit (that is changes other than adding removing files).
- I have no idea what to did to the target names in Freespace2.vcxproj.
- Please document the structure esct (or move its documentation with it).
- All files should have a newline at the end.
I do not have a problem with the use of more object orientated code because in this case it doesn't really stand out, but the general rule in that regard is to just go with what is already there.
I found it in the hudtarget.cpp file and needed to use it elsewhere, so I moved it to it's own file.
QuoteI found it in the hudtarget.cpp file and needed to use it elsewhere, so I moved it to it's own file.
Why not move it to a preexisting header file, like hud.h or pstypes.h (which is a header file defining quite a few base structs like vec3d)?
I've attached both a patch that is good against r8811 and have also attached a demo mission to help highlight the changes I've made. This patch probably breaks the cardinal rule, but I'm not sure how to go about doing anything about it, so I'll just submit the patch as-is and let the coding community give me feedback. One thing to also note: for my extensions, I utilized a mix of OO-style programming, but I'm not sure how well that will flow with a program that is primarily written in a procedural fashion. If an SCP member could give me guidance as to how to best structure and format my code so that it causes the least amount of trouble for the SCP community, that would be appreciated.
Regarding lines 187-189 of the patch. The design of evaluate_ship_as_closest_target is a very common idiom in the FSO code base. I also agree that it makes learning the code hard and can hide and/or introduce bugs into other code. However because the idiom is so common in the code base you do get used to it (as much of a cop out as that sounds).I looked at lines 187-189 (by themselves, since I don't have time ATM to review the whole patch). I agree that the design of evaluate_ship_as_closest_target is very common throughout the code and should generally be kept, but the specific issue that Mastadon raises (i.e. comparing a float to anything, even FLT_MAX) is salient. Making the one specific change of adding a bool return type would be a good design change IMO.
Personally I am not a fan of littering names all over the code because it implies ownership of the code. However, it is rather common in our code base, but it is put at the end of the comment not the beginning.Personally, my reason for leaving names is not to claim ownership, but to say "this is who to yell at if something breaks". :D However, beware the case where an unknown person rewrites the code, accidentally adds a bug, and neglects to change the name in the comment. This has happened more than once. :shaking:
However, beware the case where an unknown person rewrites the code, accidentally adds a bug, and neglects to change the name in the comment. This has happened more than once. :shaking:Indeed.
"Target closest attacker" cycles through all attackers; misc code cleanup.
-- Changed "target closest attacker" command to cycle through the other
baddies gunning for you by pressing the "target closest attacker"
command repeatedly.
-- The new functionality behaves similarly to the "target closest hostile" key.
-- Fixed bug identified where pressing the "target attacking ship" button
targets a non-attacking-but-hostile ship (Mantis issue #2656).
-- This is a bug also present in FSO 3.6.14 RC5.
-- Moved the esct (or, the "Evaluate Ship as Closest Target" struct) to
hudtarget.h, where a type declaration belongs.
-- Moved the documenting comments for hud_target_closest() to be just above
the function definition.
-- Also, converted these commments to doxygen format.
It seems as though you conflated "Target closest hostile" and "Target closest attacker" in that comment...
We're not doing an official code freeze like we've done in the past.Yeah, no kidding. At the pace we are going on this RC (I know, some of that is my fault, but look we just got wxLauncher 0.9.0 out (http://www.hard-light.net/forums/index.php?topic=67950.msg1341695#msg1341695) yesterday :D), we will need to do another release right after just to get the bugs out of trunk before the pilot code goes in.
Committed in revision: 8841
Mastatdon or a moderator: Please edit the post OP to contain [Committed 8841].
I thought the problem was that the auto-target was emulating "target closest hostile" (i.e. target my attacker) instead of "target closest enemy." Seems like it should be straightforward to just make the auto-target call whatever code the latter uses.
// ...
int valid_team_mask = iff_get_attackee_mask(Player_ship->team);
// try target closest ship attacking player
if ( Player_ai->target_objnum == -1 ) {
hud_target_closest(valid_team_mask, OBJ_INDEX(Player_obj), FALSE, TRUE );
}
// if none, try targeting closest hostile fighter/bomber
if ( Player_ai->target_objnum == -1 ) { //-V581
hud_target_closest(valid_team_mask, -1, FALSE, TRUE);
}
// No fighter/bombers exists, so go ahead an target the closest hostile
if ( Player_ai->target_objnum == -1 ) { //-V581
hud_target_closest(valid_team_mask, -1, FALSE);
}
// um, ok. Try targeting asteroids that are on a collision course for an escort ship
if ( Player_ai->target_objnum == -1 ) { //-V581
asteroid_target_closest_danger();
}
// try target closest ship attacking player
if ( Player_ai->target_objnum == -1 ) {
hud_target_closest(valid_team_mask, OBJ_INDEX(Player_obj), FALSE, TRUE );
}
Shouldn't that be ++approval;Shouldn't that have been a question? :P
;)
Seriously though, no one's ever really explained the difference to me. Is there a meaningful change in speed or something?In theory a prefix operator is faster because no data was copied (if you recall, a postfix operator returns the current value and does the increment afterwords).
Shouldn't that be ++approval;Shouldn't that have been a question? :P
;)
Seriously though, no one's ever really explained the difference to me. Is there a meaningful change in speed or something?
x = 0;
y = ++x;
x = 0;
z = x++;
After this snippet, y != z. Surprise!Question about this patch. Did this patch completely remove the ability of Target Closest Attacker to target closest hostiles if there are no present attackers?
Perhaps this patch made Target Closest Attacker now work as advertised but I feel that was done at expense of the command's original functionality which was still pretty useful.
If possible, does anyone mind if I also re-add the capability for this command to target closest hostile if there are no attackers present? I mean, if you don't have any present attackers, the command doesn't really do anything now. Might as well fill the gap.
Because you're cycling through hostiles. That key doesn't immediately give you the closest hostile exclusively. You could already be in the middle of the list of enemy targets cycled. What's wrong with a fallback behavior that emulates the original retail functionality?
That would be FoV.
One could argue that supporting higher resolutions than retail allows users to see ships further out and affects balance. :rolleyes:
2) So, since the key that cycles through Escort ships doesn't do anything with not ships in escort list, have it cycle through all friendly capships, right? I mean, the fact that your escort list is empty should tell you that there are no escortable ships... And since the target next turret key doesn't do anything after the turrets are destroyed, it should target the next nearest hostile capship and begin cycling those turrets, right? The targetted ship changing should tell you that there's no turrets left....You're thinking in a bit wrong direction here, but the general idea is good. Turret cycling key should switch to regular subsystems when no turrets are left, that'd be more logical. For the escort, cycling through friendly capships might be a good suggestion, but I'd rather go with something more like "nearest capship". It'd be good for quick orientation. Or, it could cycle through hotkeyed formations. Overall, this might become a rarely used, but convenient feature.
U no get this? Retail behaviour is changed now. It is a big deal. If something is changed, it should be kept as close tor retail as possible. While having R cycle through attackers it should also cycle through hostiles as it did in retail. So that some noobs don't happily press R and discover that this function works completely different now.
U no get this? Retail behaviour is changed now. It is a big deal. If something is changed, it should be kept as close tor retail as possible. While having R cycle through attackers it should also cycle through hostiles as it did in retail. So that some noobs don't happily press R and discover that this function works completely different now.
Kobrar44 has a good point, and my first inclination was to agree with him, but on the other hand, this new behavior could be considered the expected behavior. Suppose that if you pressed the E key to select something on the escort list, and the escort list was empty. It wouldn't make sense to target a friendly capital ship just because you might be escorting it.
So, following that line of thinking, wouldn't it make more sense to just play the "invalid keypress" error buzz, instead of targeting a hostile ship? Because if no ship is actually attacking you, then you should know about it so that you can reprioritize your gameplay.
Wait, I must be misunderstanding. We can already target the closest hostile with 'H'... what's this new command?
Wait, I must be misunderstanding. We can already target the closest hostile with 'H'... what's this new command?
#define TL_RESET 1500
That seems rather arcane. How would any new player know that you'd have to wait 1.5 seconds to get the closest enemy target when the command itself just says "Target Next Closest Hostile Ship"? That's about as "Xbox 360 context sensitive" as my previous suggestion. :P
In light of this, I still feel that "Target Closest Hostile" needs to be it's own key command for clarity. The 1.5 second wait seems even more hokey than having Target Closest Attacker falling back Target Closest Hostile when no attackers are present.
That seems rather arcane. How would any new player know that you'd have to wait 1.5 seconds to get the closest enemy target when the command itself just says "Target Next Closest Hostile Ship"?Back when I was a new player that was as instinctive to me as it gets. And until this thread, I never thought it should or even could have been otherwise.
I know this doesn't really belong in this topic.. but since we are talking about adding keybinds here it is anyway. (Feel free to split discussion if need be.)I support this!
What do people think of adding 2 or 3 null keybinds. Keys that are assigned to an action called 'Special Action' or something, but don't do anything by default. The idea here is to add a couple keybinds for FREDers to use so we don't have to keep stealing the mutliplayer keys all the time.
If you really want to improve the current targetting system, instead of f*cking around with the parts that work as intended and are perfectly fine for 99% players, one thing I think would really benefit to everyone would be a "Target closest bomber". Not the one we already have, but one that would ignore bombs, just cycle through bombers.