Skip to content

Add Sort by Available Cargo - #190

Open
dawidmachon wants to merge 2 commits into
StarCpt:mainfrom
dawidmachon:add-sort-available-cargo
Open

dawidmachon wants to merge 2 commits into
StarCpt:mainfrom
dawidmachon:add-sort-available-cargo

Conversation

@dawidmachon

@dawidmachon dawidmachon commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Adds Sort by Available Cargo.

Client-side Pulsar plugin that adds a Sort checkbox to the terminal inventory UI (right panel, next to Hide Empty). When enabled, inventory containers are ordered by remaining free cargo space (most empty first). MIT licensed.

@viktor-ferenczi

Copy link
Copy Markdown
Collaborator

Check compatibility with the recently approved https://github.com/OwendB1/se-unified-storage plugin.

@viktor-ferenczi viktor-ferenczi left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at pinned commit 2597e4fd8066f93c4d8e449dabce67fbb01b3077 (= tag v1.0.0).

Summary: manifest, security audit and from-source build all pass. One real interop defect against Unified Storage needs fixing before merge; the rest are minor polish.

Manifest ✅

python3 test.py Plugins → All files validated. SourceDirectories correctly narrows the compile to ClientPlugin, excluding the solution, docs and deploy scripts. The GUID <Id> + <RepoId> pairing is consistent with the 16 existing manifests that already use it.

No <NuGetReferences> is needed: the only package the source actually uses is Harmony, and Pulsar injects Lib.Harmony 2.4.2 itself, which matches the csproj. (The Mono.Cecil PackageReference in ClientPlugin.csproj is unused — worth dropping.)

Pin check: repo HEAD is one commit ahead of the pin, but that commit only edits the repo's own manifest copy (<Commit>scaffold</Commit> → the hash). Zero code delta, so the pinned tree is the reviewed tree. Per registry convention that in-repo copy is a backup and its <Commit> should read TODO rather than carry a hash.

Security ✅

Clean. 1834 LOC, of which ~1270 is the stock client-plugin-template settings framework.

  • No network of any kind — no HttpClient/WebClient/WebRequest/sockets anywhere.
  • No Assembly.Load*, no runtime Roslyn, no Emit/DynamicMethod, no crypto- or base64-to-execute, no string obfuscation.
  • No Process.Start or shell invocation.
  • No committed binaries — only docs/screenshot.png.
  • The two Activator.CreateInstance hits are unmodified template code over compile-time-fixed generic types, not data-driven.
  • Only file I/O is SortAvailableCargo.cfg under the game's own MyFileSystem.UserDataPath/Storage.
  • No privacy or multiplayer-abuse surface: it reads MaxVolume/CurrentVolume on inventories the terminal already displays and reorders GUI controls. Purely cosmetic, client-side.

From-source build ✅

Since the registry ignores the csproj, I replicated Pulsar's actual compile path (RoslynCompiler: C# 14, nullable context off, no DEV_BUILD, Bin64 + Harmony 2.4.2): 0 errors, build succeeded. Plain dotnet build is also clean.

Every reflected game member checks out against the decompiled source, and the accessibility story is right: MyTerminalInventoryController is internal, which is exactly why it is reached through AccessTools.TypeByName instead of a publicizer; MyGuiScreenTerminal, MyGuiControlInventoryOwner and .InventoryOwner are public, so no IgnoresAccessChecksTo is required. Signatures, control names (BlockSearchRight, CheckboxHideEmptyRight, LabelHideEmptyRight), the vanilla num = 0.004f coordinate offsets and FilterCharacter == 0 are all correct. Nicely researched work.

Unified Storage compatibility — needs a fix

Checked against OwendB1/se-unified-storage @ da09598.

Good news on layout: no collision. Unified Storage anchors its toggle to RightFilterSystemButton on the filter row (y = -0.338) and shifts the five filter buttons left; the Sort checkbox sits on the search row (y = -0.255). Disjoint. There is also no overlap in Harmony targets — Unified Storage patches Init/Close/Refresh/UpdateBeforeDraw/HandleInput/SetSearch/GetDefaultFocus, none of which this plugin touches.

The problem is functional. When Unified mode owns the right column, Unified Storage's Refresh prefix returns false, so vanilla Refresh never runs, CreateInventoryControlsInList is never reached, and CompareGuiControlInventoryOwners never fires. Two consequences:

  1. The Sort checkbox is still drawn but does nothing. It is added from MyGuiScreenTerminal.CreateInventoryPageRightSection, which Unified Storage does not patch, so the control appears regardless of who owns the panel.

  2. Toggling it corrupts the unified view. RefreshInventoryList() reflectively invokes the private CreateInventoryControlsInList and then BlockSearchRight_TextChanged. Unified Storage's prefixes sit on Refresh and SetSearch, not on those two, so both calls bypass its suppression and repopulate the shared m_rightOwnersControl with vanilla per-container controls while Unified Storage still believes it owns the panel.

Suggested fix, which also simplifies the code: in RefreshInventoryList(), call the controller's public Refresh() instead of reaching past it into CreateInventoryControlsInList + BlockSearchRight_TextChanged. Refresh() is the game's own entry point, reaches CreateInventoryControlsInList through RightTypeGroup_SelectedChanged, needs no private reflection, and — because it is what Unified Storage prefixes — composes correctly instead of bypassing it. Tradeoff: it rebuilds both columns and resets the radio selection to the persisted indices, so please check scroll/focus still behave.

It would also be worth hiding the Sort checkbox when another plugin owns the right column, so it does not present a dead control.

Minor, non-blocking

  1. 42 × CS8632 warnings in Pulsar's compile log. The Type? / MyEntity? annotations rely on the csproj's <Nullable>annotations</Nullable> and NoWarn, neither of which applies to the registry build. A single #nullable enable annotations at the top of Plugin.cs silences all of them.
  2. Localization overlap. The Sort label and checkbox use absolute X (0.285 / 0.325) while vanilla's Hide Empty label is right-aligned at 0.419. A longer localized label pushes its left edge below 0.325 and overlaps. The search-box shrink is guarded by if (searchBox.Size.X > 0.20f); these positions are not.
  3. s_sortCheckbox / s_sortLabel are never cleared, and Dispose() neither unpatches Harmony nor resets statics, so GUI controls outlive the terminal screen. - Wrong - Viktor
  4. GetTotalAvailableSpace uses break where continue is meant — an owner with a null inventory slot mid-range under-counts its free space.
  5. Non-transitive comparator. Math.Abs(spaceX - spaceY) > 0.0001f is not transitive, and SortNoAlloc → List.Sort can throw on an inconsistent comparer. It needs a contrived chain of containers within 0.1 L of each other, so the risk is low. (The KeepActiveContainerFirst double -1 asymmetry mirrors vanilla exactly and is off by default, so it is not a new defect.)
  6. LICENSE reads Copyright (c) 2026 with no name.

Everything except the Unified Storage item is author-side polish and does not block.

@dawidmachon

Copy link
Copy Markdown
Contributor Author

Hello, I will adress these. Many thanks for review. Ofc I will add compability with unified - that georgous plugin.
p.s.
didnt know about se unified inv plugin - so good idea and work there! Crazy how well could be se1 if everyone incorporate some of these ealier. Realy looking into se2, where you, soo great guys could bring sooo many other better things when MP hits.

@OwendB1

OwendB1 commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

If you need any additional guidance let us know.

@dawidmachon

Copy link
Copy Markdown
Contributor Author

Fixed at v1.1.0 (pinned commit 3352179875b6c06780af0db479679b82b73687e8).

Unified Storage compatibility — implemented your suggested fix: RefreshInventoryList() now invokes the controller's public Refresh() and the private CreateInventoryControlsInList / BlockSearchRight_TextChanged reflection is gone. Search text, Hide Empty and focus restoration are now the game's own code path (Refresh → ApplyTypeGroupSelectionChange re-applies searchBox.SearchText, SelectFirstRightInventory fixes up focus), so Unified Storage's Refresh prefix intercepts our rebuilds exactly like the game's own and the unified view can no longer be corrupted.

Dead checkbox when another plugin owns the column — the Sort checkbox (and label) now hide whenever a plugin other than ours has a Harmony prefix on Refresh() (Harmony.GetPatchInfo). One deliberate tradeoff: this detects patch presence, not the plugin's runtime mode, so with Unified Storage loaded but unified mode off, the checkbox stays hidden even though sorting would work. Runtime-mode detection was rejected because Unified Storage's mid-session vanilla→unified toggle empties the vanilla lists via Unified.Activate without any callback other plugins can observe, which would resurrect a dead checkbox after a toggle. Happy to revisit if you'd prefer mode-aware behavior.

Minor items, all done:

  • #nullable enable annotations at the top of Plugin.cs (CS8632 gone from the registry compile; the NoWarn crutch was removed too)
  • Sort label/checkbox positions are now measured from the localized label sizes — the group anchors to the Hide Empty label's left edge and the search box shrinks to fit our label (same trick vanilla uses against Hide Empty), so long localizations slide apart instead of overlapping
  • GetTotalAvailableSpace: break → continue
  • comparator: epsilon band removed (plain float comparison keeps it transitive; the KeepActiveContainerFirst asymmetry stays as vanilla)
  • LICENSE now reads Copyright (c) 2026 dawidmachon
  • unused Mono.Cecil PackageReference dropped
  • in-repo manifest copy reverted to <Commit>TODO</Commit>

Also fixed while verifying: the csproj's unconditional version defaults were silently overriding Version.Build.props (MSBuild evaluates the csproj body after imports), so the built DLL was still 1.0.0.0 after the bump — defaults are now conditional.

Thanks for the thorough review — the Refresh() entry-point suggestion made the plugin both simpler and actually composable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants