NSV13 icon indicating copy to clipboard operation
NSV13 copied to clipboard

Boarder refactor

Open Kenionatus opened this issue 1 year ago • 8 comments

About The Pull Request

Improves boarding code a bit. The addition of minimum player arguments allows to spawn boarders with a proc call on the ship, even if player numbers are below the normal minimum. Maybe I'll add a better way to call that in the future. Adds some doc comments to procs. (Putting doc comments in separate lines above vars has been deemed too inconsistent with existing code by maintainers, so that was reverted.) Makes boarders able to differentiate between guns they can fire and guns they can't when they are looking for one to pick up. Uses the same function as the gun when firing if I'm not mistaken, so should be pretty all encompassing and future proof.

Why It's Good For The Game

Doc comments make the code more readable with an IDE, as long as they don't make the code itself less readable. Moving code into functions makes code more readable. Boarders picking up guns they can't fire unintentionally weakens them in those situations.

Changelog

:cl: add: Boarders now only pick up guns they can actually fire. refactor: Adds some doc comments to procs in knpc code. refactor: Improves boarding code a bit. admin: Adds arguments to spawn_boarders proc of overmap ships. This allows to manually adjust minimum player numbers. Arguments are now "amount, faction_selection="syndicate", min_players = 5, min_players_for_ghosts = 20" /:cl:

Kenionatus avatar Jul 24 '22 11:07 Kenionatus

will cause the comments to be inconsistent with the majority of the rest of the NSV13 codebase

~~Does that mean there is an official code standard to never use above line doc comments for variable documentation in favour of end of line comments?~~

Edit: kinda got offended/scared/upset at the thought of my work not being accepted, even though it is "totally superior to the previous version of the code". I guess Code review pendingTM means I'll hear an official stance later? Also, there is a form of doc comment that should work inline //!, but it's currently broken. Not sure if it would be worth using it in the hope it'll get fixed.

Kenionatus avatar Jul 24 '22 13:07 Kenionatus

Inconstistancy with current nsv code comments is of little matter when the entire thing is that most nsv things have little autodoc functionality actually used which is very useful when properly implemented due to the huge variety of individuals working on code. Autodoc comment changes look good imo.

DeltaFire15 avatar Jul 24 '22 15:07 DeltaFire15

will cause the comments to be inconsistent with the majority of the rest of the NSV13 codebase

~Does that mean there is an official code standard to never use above line doc comments for variable documentation in favour of end of line comments?~

Edit: kinda got offended/scared/upset at the thought of my work not being accepted, even though it is "totally superior to the previous version of the code". I guess Code review pendingTM means I'll hear an official stance later? Also, there is a form of doc comment that should work inline //!, but it's currently broken. Not sure if it would be worth using it in the hope it'll get fixed.

autodoc2

/// appears to work inline

autodoc

covertcorvid avatar Jul 25 '22 00:07 covertcorvid

@covertcorvid Did you actually reload the language server? Because it doesn't work on my machine.

Kenionatus avatar Jul 25 '22 08:07 Kenionatus

@covertcorvid Did you actually reload the language server? Because it doesn't work on my machine.

I got duped, it was showing the comment from the line above

covertcorvid avatar Jul 26 '22 01:07 covertcorvid

Interesting, the bot removed the Administration label which Bookie manually added.

Kenionatus avatar Jul 26 '22 19:07 Kenionatus

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Aug 05 '22 09:08 github-actions[bot]

Currently working on doing #1951 the way it is requested, so it'll take me a while to get around to handle the feedback.

Kenionatus avatar Aug 18 '22 23:08 Kenionatus

I've started introducing error handling by returning error values (defined in nsv13\code__DEFINES\boarding.dm). It starts to feel a bit anachronistic tho (like C code). Think

    if(syndicate)
        do something
    ditto for other factions
    else
        return KNPC_SPAWN_INVALID_FACTION

The alternative I see would be to use try/catch, but that would mean even further indentation, begging me to split it up into further functions (which might not be such a bad thing...)

Kenionatus avatar Nov 22 '22 10:11 Kenionatus

Spawning zombies does currently cause a runtime because they don't have a job but the spawn code tries to give them an ID with their job on it.

Kenionatus avatar Dec 10 '22 01:12 Kenionatus