-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HITL - Networked UI system #2041
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments for reviewers.
horizontal_alignment=HorizontalAlignment.LEFT, | ||
) | ||
# Server-side message. | ||
# TODO: Handle from UI manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is currently no local fallback.
This is a planned change - the UI system should provide a minimal local implementation.
|
||
# TODO: Move to another file. | ||
|
||
# TODO: Deprecated legacy UI system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old UI elements will gradually be replaced.
These classes will be removed when all dependencies are gone.
"bottom": {}, | ||
"bottom_right": {}, | ||
"tooltip": {}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are currently hard-coded in the implementation.
This is fine for now. Later, we can make this dynamic to allow for world-space UI (VR or world-anchored elements) or other more advanced scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment here that these are hardcoded in the Unity implementation?
button: Optional[UIButton] | ||
listItem: Optional[UIListItem] | ||
separator: Optional[UISeparator] | ||
spacer: Optional[UISpacer] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ugly but does the job.
Ideally I'd like to find a better option for serializing/deserializing an union without too much boilerplate or serializer features that depend on a specific library.
A type string could be used, but it would arguably result in more error-prone boilerplate.
else None, | ||
spacer=element | ||
if isinstance(element, UISpacer) | ||
else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal.
See my comment about union serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is a bit ugly. Maybe a structure with default Nones and a map based override would be cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks sound, but hard to comment on the correctness of partial integration too deeply.
My re-phrase, we're moving from abstract objects which update cached metadata to functional calls to update the UI elements immediately. Right?
Nit: I do think the new element dataclasses should be commented to explain the variables and brief docstrings should be added to any function/class.
"bottom": {}, | ||
"bottom_right": {}, | ||
"tooltip": {}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment here that these are hardcoded in the Unity implementation?
else None, | ||
spacer=element | ||
if isinstance(element, UISpacer) | ||
else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is a bit ugly. Maybe a structure with default Nones and a map based override would be cleaner?
* Add new networked UI system. * Add UI toggle. * Add docstrings. * Consolidate UI updates per-canvas. * Add docstrings.
* Add new networked UI system. * Add UI toggle. * Add docstrings. * Consolidate UI updates per-canvas. * Add docstrings.
* Add new networked UI system. * Add UI toggle. * Add docstrings. * Consolidate UI updates per-canvas. * Add docstrings.
* Add new networked UI system. * Add UI toggle. * Add docstrings. * Consolidate UI updates per-canvas. * Add docstrings.
Motivation and Context
This changeset adds a networked UI system.
How it works
API
The UI system is retained, but the API is written in the style of immediate mode. This allows the system to work with Unity's UI, and potentially any UI framework that could be added to Habitat.
UI controls are contained within "canvases". Each canvas has a vertical layout, meaning that elements are sequentially added from top to bottom.
Elements can be given a "uid" so that they can be later referred to (e.g. to check whether a button is pressed).
Networking
When the
UIContext
scope is closed, aUICanvasUpdate
is sent to the client.clear
is true, all elements previously present in the canvas are deleted.elements
are either updated or created.The
commit_canvas_content()
method automatically sets theclear
flag if the element uids have changed.Canvases
Canvases are currently predefined in the implementation. This may change in the future to allow for, among other things, world-space canvas for VR and world-anchored elements.
The following default canvases are created:
Implementation
The UI is currently only implemented in Unity.
Example
new_ui-2024-08-26_17.44.27.mp4
Next steps
How Has This Been Tested
Tested on multiplayer HITL application with Unity clients (Unity implementation).
Types of changes
Checklist