Remote Outpost Gamemode + New Map + GM Tools #180
Conversation
Fixes a bunch of graphical and logical errors, mostly related to Whiskey Outpost. Adds new sprites. Revises some ladder and gunrack code. Adds a bunch of stuff to get ready for the Whiskey Outpost (Lowpop) mode. Adds the Blackstone Bridge map, which is a reworked WO map, as well as items, sounds, and so forth related to it. Still not done, but very close to finished. Committing work in progress in case of hardware failure. Also includes a new Game Master menu to make map-specific changes or quick destruction of the environment.
Basically complete from what I can tell. I had to do a lot of tunneling around, but all of the major features are complete: Nightmare is now fully integrated into map manipulation panel and the panel itself is complete. The map is set up, though it does need some tweaking here and there. Major work on ne squads and the ground round type. Everything works as expected from my testing, but there's still things that are not completely finished (like the specialist vendor), but I'll get to them when I get to them. A ton of bugfixes that I don't exactly remember all now. Improvements all around, from the way squads are handled, factions, to other misc things. Very rough vehicle changes to prevent them from plowing through stairs and deep water, but it's a little buggy at the moment. Refactored xeno ai to fire in the root game mode so it's not just select to the _ai game mode.
Light bug fixing and map alterations as they were still using older turf defines. Fix for initializing an empty gun rack. Removed some debug messages.
Deals with some more issues pointed out by linters and restores some multi-tile airlocks from old maps, as they are still needed.
Hopefully the last linter change, plus some minor map modifications near the mining site.
Not sure what Linters wants from me regarding lists. Hopefully this works.
Maybe this satisfied whatever cruel, malicious desire it has to ruin my comment formatting.
Not sure how it even got in there.
Fixes some doubled-up structures and adds checkpoint windows/shutters.
Looks like I missed a doubled-up table.
Fixed some bugs with the map, adjusted a few things in the code. to make other stuff happen right. Fix some other issues and runtimes I've ran into. Enabled synth and predator character preferences so that you can customize them without a whitelist (which we don't have). Synths can spawn normally on Remote Outpost, and we may have predators show up later. Increased the size of the map to be square. Can be expanded later.
Finally nails the SQ Lead issues, at least in theory. Also refactors and optimizes some squad code.
Hopefully the undeclared variable was a fluke, since everything is declared.
This comment was marked as outdated.
This comment was marked as outdated.
Lots of changes to access, factions, and so forth. Makes it possible to use the ID changer machine regardless of mode, and it is no longer hard coded to marine access. Factions are far more robust and can be used for determine subfactions. That functionality is only surface level at the moment but can be developed more. Additional fixes for RoleAuthority and squads, as well as map fixes and code fixes for the map. CM Specialist vendor works as desired now. Also makes sure that synths and yautja can be customized by players. Outside of vehicle changes, the map and associated code is almost entirely feature complete and polished.
This comment was marked as outdated.
This comment was marked as outdated.
Some more fixing and linter appeasement. Aside from some minor changes here and there, fixes mobs vaulting over south-facing barricades and appropriate ledges so that they are over the thing they are vaulting instead of under it.
A few bugfixes and a small addition, just to patch up some glaring issues. Still needs a little more tweaking.
…13-PvE-Test into Whiskey-Outpost-Mod
There was a problem hiding this comment.
Why out of 173 assignments of /obj/item/storage's can_hold list is it done in the definition, and this is the only exception now that does so in Initialize?
There was a problem hiding this comment.
https://discord.com/channels/725444629172060262/1063155296052191373/1075485560409428111
https://discord.com/channels/725444629172060262/1063155296052191373/1075485675551473674
https://discord.com/channels/725444629172060262/1063155296052191373/1075486117450764328
https://discord.com/channels/725444629172060262/1063155296052191373/1063159589509529700
As I understand it, lists should only be initialized as needed.
There was a problem hiding this comment.
Your change will make it create that list every single time any wallet is created. Whereas following the norm will mean its simply part of its definition and on byond to handle. This is just an error prone change that is unnecessary.
Revert your change, and try to keep on scope for your PR.
There was a problem hiding this comment.
You also may want to re-read the information you linked. Lummox is explaining how its preferable to do it as part of the definition. In other words its re-used data until you modify it for all other instances of that type. I see the exception you are likely trying to point out, but I don't see how what you are proposing would be any better than what already is happening.
If you really want to refactor this, do it for all, and do it in a different PR - stay on scope.
There was a problem hiding this comment.
"If your var has an init proc, e.g. var/mylist[] which will initialize a list before New() is called, or mylist = list(1,2,3), then those vars will take up more memory because they're being filled with a non-default value (in these examples, a new list unique to that object). The init proc will also take up time, and as I've mentioned elsewhere it's better to avoid init procs if you can."
"Avoid init procs wherever possible. This means something like mob/var/items[0], which not only defines items as a list but also instantiates that list whether it's needed or not. Same with mob/shopkeeper/var/wares=list(...). Init procs are time wasters and usually the same thing can be done elsewhere: either in New() or only as needed. Also any vars like this that belong to a turf will eat your memory and make the map take longer to load."
The first quote is from Lummox. Can you point out where Lummox is suggesting to do list initiation as part of the definition? I am not sure I follow what you are talking about.
There was a problem hiding this comment.
Me:
Your change will make it create that list every single time any wallet is created. Whereas following the norm will mean its simply part of its definition and on byond to handle. This is just an error prone change that is unnecessary.
- Do you disagree that Initialize is called whenever that type is new'd because SSatoms will call Initialize on it?
- Do you not see that out of 173 assignments of /obj/item/storage's
can_holdlist it is done in the definition?
Lummox:
"If your var has an init proc, e.g. var/mylist[] which will initialize a list before New() is called, or mylist = list(1,2,3), then those vars will take up more memory because they're being filled with a non-default value (in these examples, a new list unique to that object). The init proc will also take up time, and as I've mentioned elsewhere it's better to avoid init procs if you can."
- This states that performing list() in a definition will make byond execute code to create the list. This is conveyed in my message with
Whereas following the norm will mean its simply part of its definition and on byond to handle
It is completely within byond's control to better handle this situation and it very well may in the future. I also have stated that we don't know when that byond list initialization because of list creation in definition occurs; we just know that it does happen. If its shown to occur at round start when the server is loading, then it is preferable for us for it to be done ahead of time instead of during a round when someone spawns a wallet (which we do know occurs because of the object being new'd and SSAtoms Initializing it).
There was a problem hiding this comment.
This is just an error prone change that is unnecessary.
- How is it error prone? Because out of 173 assignments, this one time you do it differently.
- How is it unnecessary? Because it is out of scope for this PR, and is a peanuts change that does not matter in the large scope of things - especially for something barely used like wallets.
Do it in a separate PR, where the scope of the PR is to optimize. This will allow you and other reviewers to get empirical data on how much of a difference this even makes, and possibly when. Why? Because this PR needs to just get outstanding reviews resolved and merged rather than you keeping it in limbo with minor unnecessary changes that then need additional review. I expect you to find that the benefits you reap from changes like this are too inconsequential to be worth your time, but we can find out then with actual data.
There was a problem hiding this comment.
*Do you disagree that Initialize is called whenever that type is new'd because SSatoms will call Initialize on it?
I do believe that is how it works, yes.
*Do you not see that out of 173 assignments of /obj/item/storage's can_hold list it is done in the definition?
I haven't checked, but I will take your word for it.
*This states that performing list() in a definition will make byond execute code to create the list. This is conveyed in my message with Whereas following the norm will mean its simply part of its definition and on byond to handle
From what I am reading it is better to avoid doing what you describe. Quote: "The init proc will also take up time, and as I've mentioned elsewhere it's better to avoid init procs if you can."
*It is completely within byond's control to better handle this situation and it very well may in the future.
It may be true in the future, but I can't really theorize on what will happen. Such things are hard to predict.
*I also have stated that we don't know when that byond list initialization because of list creation in definition occurs; we just know that it does happen.
This is something that can probably be tested; presumably it runs at run time on world creation. List creation on Initialize() or similar, on the other hand, takes place exactly when you specify it takes place. To me, that seems less ambiguous.
*How is it error prone? Because out of 173 assignments, this one time you do it differently.
Those 173 assignments could be done incorrectly, or in a format that is outdated. There are sections of code that are outdated. There are also examples in the code where list initializion takes places during Initialize() or procs linked to it. We also generally don't initialize new() objects in variable definitions since it leads to all sorts of strange behavior. Just because something is done one way doesn't mean it should stay as such. Also, what errors do you foresee this change causing?
*How is it unnecessary? Because it is out of scope for this PR, and is a peanuts change that does not matter in the large scope of things - especially for something barely used like wallets.
I've already spoke about scope.
I brought this up because I thought adding this change to code guidelines going forward may be beneficial. Would you be open to get Lummox's opinion on this convo?
There was a problem hiding this comment.
You're welcome to poke Lummox about it, but Lummox is not familiar with SS13 subsystems and how we have worked around byondisms. You're better off asking in coderbus.
There was a problem hiding this comment.
Also, what errors do you foresee this change causing?
For starters, what Fira also pointed out: #180 (comment)
Where normally this list is set up before New; but now its only set up when this proc finished Initialize.
A more concrete example just randomly searching through usages of the list:
On Initialize this calls create_plushie: https://github.com/PvE-CMSS13/PvE-CMSS13/blob/f5025c63f57e3adc770c99710194bf6a8ab19641/code/game/objects/items/toys/toys.dm#L589
create_plushie calls can_be_inserted which relies on can_hold_type having can_hold, cant_hold and can_hold_skill set up already.
Had this called . = ..() after the create_plushie call it would have had no types available in the can_hold list, so it would have refused the item insertion (or maybe with no types it would have accepted anything, when it normally would have refused). Whereas before it never would have mattered. My point is this can have unforseen consequences breaking sections of code you would not have otherwise anticipated being affected.
| #define MODE_THUNDERSTORM (1<<13)// Enables thunderstorm effects on maps that are compatible with it. (Lit exterior tiles, rain effects) | ||
| #define MODE_FACTION_CLASH (1<<14)// Disables scopes, sniper sentries, OBs, shooting corpses, dragging enemy corpses, stripping enemy corpses | ||
| #define MODE_NO_XENO_EVOLVE (1<<15) // Stops all xenos from evolving or straining | ||
| #define MODE_GROUND_ONLY (1<<16) ///No ship map, ground map only. | ||
| #define MODE_XENO_AI (1<<17) ///Allows for alien AI to be used in the game mode. |
There was a problem hiding this comment.
AFAIK /// applies to the following item - if you want to document the same line as is done here you need an Inner Doc comment which is //!
|
|
||
| /atom/movable/screen/plane_master/roof_visible/relay_render_to_plane(mob/mymob, relay_plane) | ||
| . = ..() | ||
| if(. && isobserver(mymob)) /// Simple check for observers here, as that cuts down on a lot of logic that needs to be done otherwise. |
There was a problem hiding this comment.
Only use autodoc comments (///) to document procs and variables before their declaration. Any code comment should be regular ones
There was a problem hiding this comment.
It was a a frequent mistake I made. I am trying to eliminate all incorrect usage, so do note if you find more.
| /atom/movable/plane_master_controller/game | ||
| name = PLANE_MASTERS_GAME | ||
|
|
||
| /atom/movable/plane_master_controller/game/Initialize(mapload, datum/hud/hud) |
There was a problem hiding this comment.
Is there a specific reason for moving all of these to Initialize() ? /tg/ still uses regular static initialization. As far as I can see it's the same thing with added potential edge case of New-vs-Initialize
There was a problem hiding this comment.
I would figure /tg/ might refactor it already, but maybe there's something else in play. This is the conversation about why it may matter: #180 (comment)
There was a problem hiding this comment.
I don't think it's a big deal normally but I was asking for the plane master controllers specifically
During world init, SSatoms defers Initialize on creation until its own SS init (to do it in bulk). Here this means that for a duration (game init so 20-50 seconds), the value of controlled_planes won't be what it's supposed to be
From what I can review of plane master code, this shouldn't cause issues here but it's something to be careful changing for backend code, there's more to it than micro-optimizing, and frankly i don't think it's warranted touching that stuff just because
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Edit: Pretty much everything that should of been fixed has been fixed. If you plan to run Outpost mode, do let me know so I can watch for anything is behaving oddly.
Introduces a new mode, the Remote Outpost. It's a ground type game mode, similar to Distress Signal (Lowpow), but it has two custom platoons instead of one.
Introduces a new faction, the USCM Ground forces, or Outpost, that is really a part of the USCM. It does have its own telephone, OW, access, and radio networks. I did a bunch of refactoring to make this happen, as the faction and squad code wasn't quite as nimble as I would have liked.
New map, Blackstone Bridge (or Ridge, I may change my mind later), an unfinished highway and bridge through a mountain tunnel, with a nearby USCM outpost. The outpost was constructed on top of an older colony mining site. Turns out shipping goods through a jungle can attract all sorts of negative attention, such as the CLF, merc warlords, and so forth. The USCM stepped in to protect the route and make sure passing supplies weren't carrying terrorists. The planet this outpost is on isn't strictly defined yet, and you can use it for a variety of missions and scenarios. Come up with your own narrative. But the basic idea is that the minerals in the nearby mining site dried up very quickly. The USCM outpost was constructed on what remained of the settlement, including a network of tunnels underneath. Blackstone Bridge is the only way to get through the mountain range, to a presumably nearby city, and it is protected by the outpost.
You can run the map with the new mode (Remote Outpost), or in Distress Signal. I've also adjusted the modes so that xeno spawning should work regardless of the mode chosen, so you could run extended if you really wanted to (but for now it's only enabled on Distress Signal and Outpost).
The map is broken up in to several sections, which I won't detail here, but it offers a lot of different ways to entertain the players. There is a cave system, underground tunnels, points of interests, and so on. A lot of it can be adjusted on the fly, giving players access to certain paths without using the build menu.
There is a lot of refactoring done in a lot of different places. Hopefully things work, as the changes are mostly back-end. I did introduce some new graphics and sounds, but they are all fairly minor.
Big new features: Map Manipulation GM menu. It's visible under GM stuff and allows you to do three mains things.
The Map Manipulation menu probably took about as much time as the map itself, probably more.
There are also some experimental vehicle changes that I am still debugging. Prevents everything but tanks from going up/down stairs and through deep water. This can be adjusted per vehicle by GMs. There is also some code to prevent weird platform behavior, but for now it doesn't quite work the way I want to.
Should be the short of it. I'll add more if I need to.
Sounds are all sourced from pixabay: https://pixabay.com/service/license-summary/
Ported and modified vines from mainline CM repo, not a specific PR port. But the vines are available in: cmss13-devs/cmss13#5922
Ported bitfield changes from cmss13-devs/cmss13#5369