Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ships to arrive from hangars on player ships #369

Merged
merged 9 commits into from Oct 24, 2015
Merged

Allow ships to arrive from hangars on player ships #369

merged 9 commits into from Oct 24, 2015

Conversation

The-E
Copy link
Member

@The-E The-E commented Oct 3, 2015

A longstanding issue for capship command missions has been that hangars on player ships don't work. The reason for this was that the engine kept the player ship in the Arrival_List; according to the :V: comments, this was done to allow respawns in multiplayer.
I've amended the check so that the player ship gets treated correctly in singleplayer.

@Goober5000
Copy link
Contributor

The new change ought to have comments. Add one to the function header that explains why returning NULL is the right thing to do (for any and all functions that might call it), and add another comment to the if() block that explains how the conditions work.

@The-E
Copy link
Member Author

The-E commented Oct 5, 2015

Added the requested comments.

@Goober5000
Copy link
Contributor

Looks good!

I was about to approve the merge, and then it occurred to me that we should test that respawns still work in multiplayer. Could you test that, or alternatively could you trace through the code logic to make sure that respawns don't depend on this function returning the old result?

@The-E
Copy link
Member Author

The-E commented Oct 6, 2015

Yeah, I should see if I can get some testing done on this. That said, this is why I put the call to ship_name_lookup in there; As I understand it, that function should return -1 if the ship being looked up is not currently present in-game (which should be the case for players waiting for respawn).

* Returns the parse object on the ship arrival list associated with the given name.
* Returns NULL if the object is not on the Arrival list, or if the object is a player
* ship that is already in game (Player ships never get removed from the arrival list
* in order to make respawns in multiplayer work).
*/
p_object *mission_parse_get_arrival_ship(const char *name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given touching this function and mission_parse_get_arrival_ship(), would suggest you provide the function documentation with Doxygen features. e.g. https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html

@The-E
Copy link
Member Author

The-E commented Oct 8, 2015

Added comment tweaks.
In addition, a look through the player death code shows that player objects get changed to the OBJ_GHOST type on death, this will cause ship_name_lookup to return -1 if the player is currently awaiting respawn, which will make this work just as it did before.

@MageKing17 MageKing17 added the enhancement A new feature or upgrade of an existing feature to add additional functionality. label Oct 12, 2015
@MageKing17
Copy link
Member

Nothing else jumps out at me.

@asarium
Copy link
Member

asarium commented Oct 13, 2015

I noticed you use \brief instead of @brief and also kept that style for the rest of the comments. Any specific reason you did that? The rest of the code uses the @ variant of the doxygen comments.

@The-E
Copy link
Member Author

The-E commented Oct 13, 2015

I was looking at the comments in the hud code for guidance. Easy enough to fix.

@@ -3967,7 +3975,8 @@ int parse_wing_create_ships( wing *wingp, int num_to_create, int force, int spec
wingp->flags |= WF_WING_GONE;

// if the current wave is zero, it never existed
wingp->flags |= WF_NEVER_EXISTED;
if (wingp->current_wave == 0)
wingp->flags |= WF_NEVER_EXISTED;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this affect any retail missions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll revert the change and change the comment instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the use of WF_NEVER_EXISTED should be audited to see if its name matches its function, as I have a sneaking suspicion that it doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WF_NEVER_EXISTED does match its function, but it is only checked in one place -- the sexp_query_has_yet_to_arrive helper function -- in a way that appears redundant.

Actually, this situation is rather complicated and carries some interesting consequences. See #379.

MageKing17 added a commit that referenced this pull request Oct 24, 2015
Allow ships to arrive from hangars on player ships
@MageKing17 MageKing17 merged commit 1ba35d4 into scp-fs2open:master Oct 24, 2015
@The-E The-E deleted the player-fighterbays branch August 12, 2016 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or upgrade of an existing feature to add additional functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants