fix startpos cold loads on maps with dust, clutter, and lightning - #140
fix startpos cold loads on maps with dust, clutter, and lightning#140Microck wants to merge 4 commits into
Conversation
DustEdges creates dust-noise-a/b in BeforeRender, so a never-rendered fresh baseline holds null at the anchor owner field, and exiting the map disposed the captured level instances out of VirtualContent. A cold load of a slot saved near dust bunnies then failed every lookup and refused the slot. Restore now recreates the equivalent wrapper as a last resort: only for label keys, only after the fresh index, the detached registry, and the structural path all miss, and never at capture. The changelog contract guard now checks the newest contract mention instead of demanding one in every active section.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: Warning Your free Security trial is over. An organization admin can activate billing to continue. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a77a97799
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| string name = resourceKey.Substring(0, dimensionsSeparator); | ||
| if (name.IndexOf('/') >= 0 || name.IndexOf('\\') >= 0) { | ||
| return null; |
There was a problem hiding this comment.
When a lazily loaded file-backed texture has a bare path such as icon.png, this separator check classifies the path as a runtime label. GetLiveResourceKey accepts any nonempty VirtualTexture.Path, and all virtual assets are marked non-portable, so if the fresh graph and detached registry miss that texture during a cold load, this branch creates a transparent texture and reports a successful restore instead of preserving or refusing the real content. Restrict recreation to the known dust-noise labels or carry explicit capture-side evidence that the texture was data-backed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fair. recreation is now an exact allowlist (dust-noise-a/b only), so a bare file path like icon.png keeps the refusal. new populations get added on measurement instead of guessed from the key shape. 82b2ada
| // 16384 is the largest texture current GPUs accept, and a real | ||
| // runtime-built texture is a fraction of the screen; the cap keeps a | ||
| // hostile snapshot from demanding the allocation instead. | ||
| if (dimensions.Length != 2 || | ||
| !int.TryParse(dimensions[0], NumberStyles.Integer, CultureInfo.InvariantCulture, out int width) || | ||
| !int.TryParse(dimensions[1], NumberStyles.Integer, CultureInfo.InvariantCulture, out int height) || | ||
| width <= 0 || height <= 0 || width > 16384 || height > 16384) { |
There was a problem hiding this comment.
When a snapshot is corrupted or edited to carry a valid runtime-texture key such as dust-noise-a|16384x16384, this guard permits an approximately 1 GiB RGBA allocation, and distinct keys can request several such textures during one restore. Thus the newly added hostile-input guard can still exhaust system or GPU memory and crash the game; validate the width-height product against a small byte budget appropriate for generated textures rather than using only the GPU's maximum dimension.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
done, the guard is now a pixel budget (512x512, 1 MiB rgba) instead of the gpu max per axis, so a doctored key cannot demand the allocation. 82b2ada
A bare file path is indistinguishable from a made-up name in the key, so recreation is now limited to the measured dust-noise labels, and the dimensions are bounded by pixel count instead of GPU maximums so a doctored snapshot cannot demand a 1 GiB allocation. Both from PR review.
Cold loads that survived the dust-noise fix still refused rooms whose saved graphs hold state a clean room load never builds. Five licences close what the six-map verification protocol surfaced: - hash-container entry indices (HashSet/Dictionary _entries) wildcard in structural keys like list indices do; they are per-process facts and Celestial Resort clutter graphs run through them - vanilla runtime entities get the generated-entity owned-state licence, so a surplus clutter block carries its own collider - a Component built on first use and kept in a declared field no ComponentList carries restores into its proved owner (DustGraphic blink, Bolt routine), never when disposable and never over a fresh object - a runtime entity keeps its captured reference to the fresh component that built it (DustGraphic Eyeballs) - an owner-proved iterator canonical stack edge is licensed when the fresh routine already finished, and owned-nested-state owners count as iterator owners (LightningRenderer bolts) Verified in game per map on the test box: 3 slots in different rooms, warm load each, exit, re-enter, cold load all 3 - Celestial Resort, Summit, Farewell, SC2020 Cookie, SJ hyperlife, 9D Resort D-side, 36/36.
Microck
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 334cb0b95c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| string[] dimensions = resourceKey.Substring(dimensionsSeparator + 1).Split('x'); | ||
| if (dimensions.Length != 2 || | ||
| !int.TryParse(dimensions[0], NumberStyles.Integer, CultureInfo.InvariantCulture, out int width) || | ||
| !int.TryParse(dimensions[1], NumberStyles.Integer, CultureInfo.InvariantCulture, out int height) || | ||
| width <= 0 || height <= 0 || (long) width * height > MaxRecreatedTexturePixels) { |
There was a problem hiding this comment.
Fresh evidence after the prior per-texture fix: the allowlist constrains only the label, while arbitrary distinct dimensions such as dust-noise-a|512x512, dust-noise-a|511x513, and many more all pass this check. A doctored .akr can include many such anchor nodes, causing up to 1 MiB to be allocated for each before graph validation completes; VirtualContent retains those textures even if a later node makes the restore fail, so this can still exhaust memory. Require the known 128x72 dimensions for these two labels or enforce a cumulative recreated-pixel budget for the entire restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch, the label alone still let made-up sizes mint distinct keys. the table now pins name and size together (DustEdges hardcodes 128x72 for both), so one process can only ever materialize those two wrappers, and a repeat key reuses the registered one. 2f4e380
The allowlist bounded the label but not the allocation: distinct made-up sizes each minted a fresh key and a fresh 1 MiB texture, and a failed restore leaves them registered. DustEdges hardcodes 128x72 for both noise textures, so the table now pins name and size together and one process can only ever materialize those two wrappers. From PR review.
1 participant
what
startpos slots on several maps wouldnt load after you left the map and came back. this started as one bug (dust bunnies) and turned out to be a stack of them, each hidden behind the last: the token ceiling from #138 was refusing these loads before they could reach any of this. this PR fixes the whole stack that a 6-map verification protocol surfaced.
the layers
each fix exposed the next one, so they're listed in the order they were found:
DustEdgesbuildsdust-noise-a/blazily inBeforeRender. the fresh baseline a cold load pairs against is never rendered (null slot, no structural path) and exiting the map disposed the captured level's instances out ofVirtualContent. every lookup missed and the slot was refused. the restore now has a last-resort recreate step for an exact allowlist of runtime texture labels, bounded by a pixel budget, authenticated by the same key comparison the detached lookup uses. capture never recreates anything.ClutterBlocks cross-reference each other throughHashSets. hash entry positions belong to the saving process, so structural keys now wildcard_entriesindices the way they already wildcardList._items, and vanilla runtime entities get the same owned-state licence generated mod entities had (a surplus block carries its ownHitbox).DustGraphicbuilds ablinkcoroutine and anEyeballsentity when dust is on camera. neither is in any component list, so nothing could vouch for them. a component built on first use and kept in a declared field now restores into its proved owner (never whenIDisposable, never over a fresh object), and a runtime entity keeps its captured reference to the fresh component that built it.LightningRenderer'sBoltruns its own coroutine by hand. a saved mid-flight routine against a fresh bolt whose routine already finished has no fresh occurrence to spend, which is expected silence, not missing evidence. an owner-proved iterator's canonical stack edge is now licensed, and owned-nested-state objects count as iterator owners.all of these are pre-existing (the resolver code was untouched since beta 70); the token-cap fix just made them reachable.
test plan
scripts/akron-verify/run.shharness at 0 failed checks on the final build (cross-room/cross-map/restart pixel-hash equality, load latency, log gates).known limits found along the way
planning and implementation with Claude (Fable), reviewed and tested by me on the remote test box.