3mo xeno archeology (first phase) #33370
Conversation
There was a problem hiding this comment.
Right, OK, I've at least gone through 85% of this. I got about as far as the ftl files and skimmed the node generation and traversal code, but otherwise got through everything.
This is like, six different PRs in one; it's a full rebuild of xenoarch that can't live alongside the current xenoarch codebase.
I've gone through and found a lot of codebase nitpicks whilst trying to understand the code, but I think there's simply way too much here for one PR.
It's not helped by the really large amount of systems and code in general. There's a lot of systems for individual types of triggers and effects; I can't help but think a partial class-based approach would be much more readable.
I'll start on actually testing this out tomorrow so I can give UI/UX feedback and generally see if this works out without being RNG hellscape.
OVERALL COMMENTS FROM JUST READING IT:
- The "XAT/XAE" jargon is not helpful and doesn't match normal Wizden conventions. "Xenoarch THING Effect/Trigger" is easier to read.
- There's a tonne of systems here that do very small amounts of work individually. Feels a bit redundant. Depends on Wizden conventions tho.
- Lots of small code quality quibbles, they can entirely be ignored.
- This really needs a better strategy to get broken out. I get that Wizden merges are pulling teeth, but this needs a maint to sit down and be able to give it an updoot, and at the moment it took me, a lowly contributor, nearly four hours to eat through, and I've not even checked this code other than compilation and a smoke check yet.
On 4: you're asking for a maint to probably spend at least a day on going over this. I think the best strategy for this is figuring out how to sidecar this into the current codebase and roll out expansions to it to add more effects, before swapping out the current Xenoarch. That shouldn't involve going backward.
Realistically getting this down to like, one trigger, one effect, etc, and worrying about content afterward would be where I'd start.
| namespace Content.Client.Xenoarchaeology.Equipment; | ||
|
|
||
| /// <summary> | ||
| /// This handles... |
There was a problem hiding this comment.
Remove redundant docstring (or explain what this class does)
There was a problem hiding this comment.
missed it, sry :3
| if (!TryGetAnalysisConsole(ent, out var analysisConsole)) | ||
| return; | ||
|
|
||
| if (_ui.TryGetOpenUi<AnalysisConsoleBoundUserInterface>(analysisConsole.Value.Owner, ArtifactAnalyzerUiKey.Key, out var bui)) |
There was a problem hiding this comment.
DRY, this is the same work as lines 26-27 using a different entity so make a method.
| { | ||
|
|
||
| } | ||
| public sealed class ArtifactCrusherSystem : SharedArtifactCrusherSystem; |
There was a problem hiding this comment.
Avoid unneccesary file changes in an already chonky PR.
There was a problem hiding this comment.
sign fine... but please do notice that it took me 20 minutes to enforce remove on last line to perfectly revert changes due to (i think) editor config defaults...
| _audio = _ent.System<AudioSystem>(); | ||
|
|
||
| if (BackPanel.PanelOverride is StyleBoxTexture tex) | ||
| tex.Texture = _resCache.GetTexture("/Textures/Interface/Nano/button.svg.96dpi.png"); |
There was a problem hiding this comment.
Apparently just calling random paths is a thing in SS14 UI code but magic strings being elevated up to STATIC_CONST is commonly cleaner.
There was a problem hiding this comment.
Apparently just not asserting existence of the file and just going "whoop-dee-doo I guess we'll do something arbitrary" is also common in SS14 UI code :death:
Not your fault; the rot consumes
|
|
||
| var comp = _ent.GetComponent<AnalysisConsoleComponent>(owner); | ||
| _owner = (owner, comp); | ||
| Update((owner, comp)); |
There was a problem hiding this comment.
Generally avoid cuddling alloc (rows that contain =) with non-alloc work. A whitespace denotes a change in activities.
var foo = "hello world";
Debug.Log(foo);
is fine, but for any more than one alloc de-cuddle.
var foo = "hello world";
foo = $"{foo}!";
Debug.Log(foo);
There was a problem hiding this comment.
i see that i can use _owner to pass into Update in this line, but overall i'm not 100% sure i understand what you are talking abou there. dotnet have a lot (and i mean A LOT) of tools to optimize code to make stupid shit developers do run better without making them drop readability down. And creating reference for object on current method stack is like 100% not the thing that really worth optimizing Q_Q
| /// This is used for an artifact that is activated after a certain amount of damage is dealt. | ||
| /// </summary> | ||
| [RegisterComponent, NetworkedComponent, AutoGenerateComponentState, Access(typeof(XATDamageSystem))] | ||
| public sealed partial class XATDamageComponent : Component |
There was a problem hiding this comment.
Good example of why these need renaming. If you don't read "XAT" this is just a damage-tracking component, not something that triggers something.
XenoarchDamageTriggerComponent is much clearer.
There was a problem hiding this comment.
this name is bad - true. but.. Unified way of naming actually helped a lot duing development, because whole start of class is just reamarks 'is taht what u are looking for'.
For now i'll rename it XATDamageThresholdReachedComponent
| public sealed partial class XATTimerComponent : Component | ||
| { | ||
| [DataField, AutoNetworkedField, AutoPausedField] | ||
| public TimeSpan NextActivation; |
There was a problem hiding this comment.
Only a regular interval and not a random one?
There was a problem hiding this comment.
don't know why Emo left it this way. Maybe better to let it be min-max too?... gonna look into this...
There was a problem hiding this comment.
fixed that thingy, a bit at least...
| var targetCoords = Transform(args.Target).Coordinates; | ||
|
|
||
| var query = EntityQueryEnumerator<XATDeathComponent, XenoArtifactNodeComponent>(); | ||
| while (query.MoveNext(out var uid, out var comp, out var node)) |
There was a problem hiding this comment.
There's quite a few "if this ever happens, uh, try and trigger this even if it's very far away" effects. This could be messy in some extreme cases.
There was a problem hiding this comment.
Elaborate, pls... is this actionable or just the idea of something bad happening in XAT systems?
There was a problem hiding this comment.
Any more details there? >_> or ideas?
| throw new ArgumentException($"node {ToPrettyString(node)} is not present in {ToPrettyString(ent)}"); | ||
| } | ||
|
|
||
| public bool TryGetIndex(Entity<XenoArtifactComponent?> ent, EntityUid node, [NotNullWhen(true)] out int? index) |
There was a problem hiding this comment.
Document public API
There was a problem hiding this comment.
done, all of shared methods received loads of xml-doc
| if (ent.Comp.NodeVertices[index] is { } netUid && GetEntity(netUid) is var uid) | ||
| return (uid, XenoArtifactNode(uid)); | ||
|
|
||
| throw new ArgumentException($"index {index} does not correspond to an existing node in {ToPrettyString(ent)}"); |
There was a problem hiding this comment.
Seeing a throw here without much in the way of handling above it. A tad spooky.
There was a problem hiding this comment.
well, i honestly am not sure why its not try-get pattern, i was considering this a problem - but during testing i was REALLY not excited of scenario when some code was adressing invalid index and game would just go along with it and say 'okay, i cannot do that, here is a log that you never going to see, bye!'
I obviously can remake it into try-get, with loads of additional code that will be enforced to make additional checks... i am just not sure i really want that...
@FairlySadPanda i was not considering breaking this into parts for several reasons:
All in all, i am too, kinda sad about that PR eating away too much time on review, i spent 4 man-days in sum to just review original Emo work (i was considering if changes are required on every little thing). But i have no idea how to make it better without hurting something in exchange. |
|
About XAT/XAE convention - we already have old naming convention and as i understand, XAT-XAE was specifically made to be destinguished and not be confused... It honestly makes proto easier to write and read. Other then that i cannot say anything in defence of an idea. I don't think it would be too much work to change it all, if we are confirming it is needed. Maint opinion pls! @chromiumboy ? :D |
|
I do think it's a bad naming scheme but breaking the PR apart is probably more important I might have an idea for this fwiw, it just needs orienting around a feature flag |
|
remarks after playtest with 10+ wizden boiz and galz:
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
S: Merge Conflict
… irradiate themselves)
…iltered properly now, rad dmg trigger now requires only 20 dmg for activation
github-actions
bot
added
size/L
github-actions
bot
removed
the
S: Merge Conflict
…finish of unlocking phase
… triggers player activated correctly. Failed one stops incrementing
github-actions
bot
added
Changes: Sprites
About the PR
Re-doing of xeno archeology, mostly done by legendary EmoGarbage.
This PR implements part of https://docs.spacestation14.com/en/space-station-14/departments/science/proposals/xenoarch-redux.html and creation of pathway to fully implementing this design doc, and more possibilities!
Stuff that PR is addressing:
Stuff that PR is not implementing:
job done:
Side effects:
Why / Balance
Implementation of design doc to make archeology more involving, less random, introduce richer system for interactions with artifacts.
Technical details
Media
https://www.youtube.com/watch?v=e-i975ANjNA
Requirements
Breaking changes
All old Artifact-node related components, systems and yml were changed, moved or deleted. New stuff is not 1-to-1 compatible.
Changelog
🆑 EmoGarbage404, Fildrance