Conversation
kipdotnet
left a comment
There was a problem hiding this comment.
* [x] I have added media to this PR or it does not require an in-game showcase.
Please dont check this box if you havent added media. This PR is significant enough to need a media showcase
|
This implementation had a bit of a frame hitch when grabbing the ruins. Updated to hopefully fix that and add an image to showcase. |
|
It's worth noting that this in its current version spawns it a bit far away from the station, and spawns bigger ruins. But those are both parameters controlled via YAML if people want it to spawn smaller ruins or spawn them closer so salvage has an easier time getting to them. |
|
|
I thought it was supposed to be '_Macro'. Well crap. Fixed. Also Github hates renaming folders |
mqole
left a comment
There was a problem hiding this comment.
hefty review. sorry
in general, im definitely still a bit ignorant on what lotsa this does, but my general observations are that the cs should definitely be broken down a lot more. its a touch spaghetti in there and using helper methods to break some of this code up would do a lot to make it easier to read.
im not too familiar with the use cases here (uploading map files to make ruins out of i assume? only put that together at the end oops) but it does seem like a bad idea to have so much about what prototypes are defined as what tiles being hardcoded. it would be really nice if there was the potential for people to have an easy time modifying the list of what is considered to be in each category.
take all my other comments with spoonfuls of salt
There was a problem hiding this comment.
this all needs comments, & please put macro usings together as well
| break; | ||
| // Macro start - Adding RuinOffering to the salvage system | ||
| case RuinOffering ruin: | ||
| option.Title = Loc.GetString("salvage-magnet-ruin"); |
There was a problem hiding this comment.
asking someone smarter than me. how do we feel about hardcoded string here. is it worth
| using Robust.Shared.Serialization.Markdown.Value; | ||
| using Robust.Shared.Utility; | ||
|
|
||
| namespace Content.Server.Salvage; |
There was a problem hiding this comment.
| namespace Content.Server.Salvage; | |
| namespace Content.Server._MACRO.Salvage; |
| [Dependency] private readonly IPrototypeManager _prototypeManager = default!; | ||
| [Dependency] private readonly ITileDefinitionManager _tileDefinitionManager = default!; | ||
| [Dependency] private readonly MapLoaderSystem _mapLoader = default!; | ||
| [Dependency] private readonly ILogManager _logManager = default!; |
|
|
||
| /// <summary> | ||
| /// Cached data for a map, including cost map, coordinate map, wall entities, and window entities. | ||
| /// </summary> |
There was a problem hiding this comment.
can you add parameter summaries for this summary block?
There was a problem hiding this comment.
remind me to go over these comments a bit later with a fine tooth comb. i definitely want to make sure there is some more clarity on the minutae of what is being changed because i am insane from doing upmerges, but i also dont want to step on toes & this is getting to be a chunky review lol
| public abstract partial class SharedSalvageSystem | ||
| { | ||
| private readonly List<SalvageMapPrototype> _salvageMaps = new(); | ||
|
|
There was a problem hiding this comment.
if you must remove this then just comment it out
| # Ruin maps that can be used for salvage magnet ruin offerings. | ||
| # These maps are used to extract floor and wall tiles using flood-fill. | ||
| # Recommended to keep the total number of maps 5 or less. More maps will slow server startup. Make sure the maps selected are mostly station-like | ||
| # Avoid any stations that have large amounts of asteroid walls or xeno-walls. The ruin system was not designed to parse those types of maps. |
There was a problem hiding this comment.
yeah this is what i mean with like, allowing people to define their own walls. makes for a quick and easy customisation
|
|
||
| - type: salvageMagnetRuinConfig | ||
| id: Default | ||
| floodFillPoints: 25 #if you increase this, also increase the ruinSpawnDistance, otherwise the ruins will spawn ontop of the magnet |
There was a problem hiding this comment.
it would be nice if cs for distance had some clamps so the computer did the work of increasing distance for you instead of having to do it manual
About the PR
Added the Salvage Ruins system that has been live on Funky for several months.
This is intended to replace the Debris magnet option, but nothing about this breaks the debris system, it's simply disabled in the YML config of the magnet.
Why / Balance
Salvage Debris are boring RNG of random walls and floors. This system makes it much more dynamic looking, and makes it look like actual pieces of space stations.
There are a number of YML settings to configure the ruins to either spawn smaller, or have better/worse loot, or to use your own specific server's maps for the ruins. It reloads the map on server startup, so if a downstream repo changes the ruin_maps.yml to their own specific server's maps, it will make ruins that look like their own server's destroyed stations. And stay up to date automatically with any map updates.
Technical details
Calling out here that this code IS slightly different from Funky's salvage magnet code. Funky's code also redefines the loot table mechanics a lot, and I left that out here. I also had to change a bit of the back end code about how it generates due to Funky's version still being based on our old pre-Macrocosm codebase. Just wanted to call it out.
A lot was done on the back end to make this performant and not lag the server. Here's the path flow for how the code works:
This system was 100% created by me, and I specifically am licensing it under MIT for my effort porting it here to Macrocosm.
Media
Requirements
Breaking changes
Changelog
🆑 Terkala