[Mastadon]

Hi everyone.

I had a bit of inspiration today and extended the "target my attacker" key so that it cycles through all of the ships that are attacking you rather than just the closest attacker---much like the "target nearest hostile" key works. This change allows you to, for example, ignore the Aten-class cruiser hanging 500m off your starboard bow w/ a destroyed weapons subsystem and see the wing of Thoths that are 800m away and trying to gun you down.

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.

Thanks in advance!

[attachment deleted by a ninja]

[X3N0-Life-Form]

After spending a couple minutes trying to figure out something appropriate, I believe I'm gonna go with a simple "hell yes".
So, here it is:

Hell yeah \o/ .

[niffiwan]

Umm - your patch removes a number of recent commits from trunk, perhaps from around 8800 onwards?    Could you please redo your patch to just include your targeting changes?

As for the patch itself, cool idea, I'd definitely find that feature useful 

[Iss Mneur]

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.

[Capt_Thunder]

Thank you. This has been needed for a long time.

[Dragon]

I agree, this would be a great change. I think it could be on by default, it doesn't break anything retail and should improve gameplay in a subtle, yet noticeable way.

[Mastadon]

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.

Thanks for the in-depth feedback Iss Mneur: it is appreciated. I will try and answer your feedback in the order in which you gave it:
  - I overlayed a mercurial repository over my working copy of fs2_open and have been mirroring changes back into the "default" branch (mercurial's equivalent to "trunk"), keeping my various patches in separate branches. What happened here is I forgot to merge the default branch into my tragetting_key_fix branch, resulting a bunch of superfluous changes when I created the svn patch. I've since corrected that oversight.
  - I've adjusted my file naming conventions to be "two_word_name" compliant.
  - I'll change the comments to doxygen-style in a later update...possibly tonight.
  - Pragmatically speaking, I did nothing of consequence to the Freespace2.vcxproj. Internally, I tacked on a "_targetting_key_fix"suffix to the FS2 open binaries generated so that I can easily keep track of my various patches and their compiled products. I've since stripped out any "changes" to the Freespace2.vcxproj.
  - I really am not sure what the esct structure does. I found it in the hudtarget.cpp file and needed to use it elsewhere, so I moved it to it's own file. I can do an svn blame on hudtarget.cpp to try and find out where the esct struct came from, but judging from the code I edited, it was probably in the original codebase.


[attachment deleted by a ninja]

[The E]

I 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)?

[Mastadon]

I 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)?

Because at the time, I wasn't sure where to put such a struct, so I defaulted to my standard practice of declaring and / or defining one class / struct / enum per file. However, when I get to work on triaging the other issues that have been raised about this patch, I'll move the esct struct over to hudtarget.h, since the struct was originally defined in hudtarget.cpp and it would make the most sense to move it to it's associated header file, where type definitions belong.

[Goober5000]

Excellent work Mastadon, and your efforts are appreciated.   Keep this up and we might make you an official SCP member.

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.

In answer to your questions...

1) The cardinal rule, as you have read, is that gameplay on FSO should be the same as gameplay on FS2.  Generally this applies to things like ship/weapon balance and AI, not so much the interface.  This new behavior ought to be acceptable, as it sounds very much like the existing "target nearest hostile" key behavior.  (I'm speaking in theory as I haven't tested it yet.)

1a) When coding new features that affect balance and AI, the retail behavior is usually made a special case of the new behavior.  For example, ship-to-ship docking is now a special case of multiple ship docking.

2) The rule of thumb is the modifications and edits to existing features should be written using the same techniques and styles.  (It is jarring when an otherwise consistent bit of code has a sudden change midway through.)  But if you design a new feature, or significantly rewrite an existing feature, you can change it to your own style.

3) Commits and patches should usually focus on one thing, or one small group of closely related things.  Cramming a bunch of unrelated changes into a single commit is heavily discouraged as it introduced lots of bugs during SCP's earlier days.

4) Be cognizant of potential pitfalls for other platforms.  Mixed-case file names will trip you up on Linux.

5) What IssMneur said.

[Mastadon]

Ok. I believe I've made all the changes Iss Mneur and Goober5000 recommended. I've attached the current version to the first post, so as to avoid confusion as to which version of the patch is the current version.

[Goober5000]

Cool.  I think this is an excellent candidate for integration with trunk... all it needs is a code review by a SCP member.  (To be fair, we should probably also wait for the 3.6.14 release.)

Bump this (and your other patch thread) in a week or so to remind people about it.

[Mastadon]

Will do. 

[Iss Mneur]

The patch itself appears to be sound, just some points of style that are bothering me before I put this in trunk.

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).

Okay, now to pick on the style:

Hard wrap your lines at 80 chars (that is, use the enter key to wrap the lines).  I have a 19in monitor and some of them still run off the the screen when my diff viewer is full screen, I shudder to think what would happen in the the tiny window for code that results when debugging.

If you don't know how to set your favourite editor to at least show a line at 80 columns I am sure one of us SCPers (or google) would be able to help you.  If your favourite editor cannot show a line at 80 columns, I suggest you get a better editor.

Please fix the extraneous whitespace (and inconsistent use of whitespace) for examples:

• Chunk starting on line 5
• Chunk starting on line 13
• The code block that starts on line 21 introduces a number of whitespace errors (tabs or spaces without non-whitespace after them)
• Why is the comment indented? line 26-36
• whitespace after the colons on line 55 (not counting that they should be wrapped at 80 columns)
• there should be no whitespace inside [] on line 56
• Chunk starting on line 213
• Why are the comments indented on line 362
• if you are going to move code, fix the whitespace at the same time 361-373

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.

Finally, you haven't included a commit message.   Preferably a <60 char summary or a <60 char summary with a longer explanation after.

[Goober5000]

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".   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.

[Iss Mneur]

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.

Indeed.

This is why we have svn blame or svn praise. You cannot lie to the version control system. No relying on the honour system that way.

[chief1983]

You _can_, however, inadvertently claim code via whitespace or typo refactoring, which is why I tend to discourage that kind of refactoring once code is in place.   Makes for a confusing history.  Get it right the first time

[Mastadon]

Ok, I've gone ahead and fixed the various code formatting issues and have replaced the patch w/ version 4 of the cycle attakers patch.


Here is a commit message that summarizes the changes this patch makes to the codebase, complete with a one-line summary:

"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.

[Goober5000]

It seems as though you conflated "Target closest hostile" and "Target closest attacker" in that comment...

[Mastadon]

It seems as though you conflated "Target closest hostile" and "Target closest attacker" in that comment...

D'oh! I must have made that error when I originally wrote the local commit log entry about a week ago. In any case, I've fixed that oversight.