Repository navigation
DOCS: Replacing architecture diagrams - #2448
suearkinunity wants to merge 17 commits into
Conversation
|
|
suearkinunity
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2448 +/- ##
============================================
- Coverage 78.96% 58.32% -20.65%
============================================
Files 767 738 -29
Lines 140949 136505 -4444
============================================
- Hits 111305 79610 -31695
- Misses 29644 56895 +27251
Flags with carried forward coverage won't be shown. Click here to find out more. |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Harness Review
The new Mermaid diagrams make the architecture more explicit, but the runtime-flow description now misrepresents how action callbacks and action-state data are connected to the implementation.
Reviewed commit 8efe30b
🤖 Helpful? 👍/👎
| space -->|"state change monitor"| ctrl | ||
|
|
||
| %% ---------- Callback out to the scene ---------- | ||
| trig -->|triggers| IU["InputUser"] |
There was a problem hiding this comment.
InputUser is not the intermediary that invokes PlayerInput.OnActionTriggered(). PlayerInput installs its OnActionTriggered delegate directly on each InputActionMap.actionTriggered event; InputUser manages user/device/action association. As drawn, a reader tracing a normal PlayerInput action will look for a callback route that does not exist. Replace this edge with the action/map callback flowing directly to PlayerInput (and present InputUser separately as the device-pairing layer).
🤖 Helpful? 👍/👎
| %% ---------- Action state ---------- | ||
| AState(["InputActionState | ||
| NotifyControlStateChanged()"]) | ||
| trig["triggerStates[]"] |
There was a problem hiding this comment.
This diagram and the preceding text name a triggerStates[] array, but InputActionState actually exposes actionStates (whose element type is TriggerState), alongside bindingStates and controls. Also, state-change monitors call into ProcessControlStateChange; they do not update the binding/control arrays shown by the incoming arrows. This makes the new architecture diagram impossible to reconcile with the source and incorrectly suggests binding resolution happens for every state event. Rename the action-state array and draw the monitor as invoking action-state processing while the resolved binding/control arrays are read from it.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
yeah probably worth pasting this into whatever generated the diagram and telling it to regen
K-Tone
left a comment
K-Tone
left a comment
There was a problem hiding this comment.
I love this change, however one of the diagrams renders a little bit internally crowded, with elements overlapping each other in a few spots. Could you check if that is on my end (caused by rendering Mermaid in SoloMD), or is it really going to be rendered that way for users?
|
Sure thing - let me rebuild the preview |
|
@K-Tone please check again |
|
@K-Tone please can you also check u-pr's comments? |
K-Tone
left a comment
K-Tone
left a comment
There was a problem hiding this comment.
looks good, thanks for addressing feedback
|
https://github.cds.internal.unity3d.com/unity/unity/pull/130973 for further work (against Trunk) |
2 participants
Description
manual/Architecture.html has two old diagrams. They're very hard to read, and we've had user complaints about that.
This is an attempt to replace them with Mermaid diagrams.
Testing status & QA
Docs only
Overall Product Risks
None: docs only
Comments to reviewers
Very different way to construct the diagrams, so requires careful review.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.