-
Notifications
You must be signed in to change notification settings - Fork 16
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
Player components and Comms extensions #144
Conversation
Test this pull request
|
Squashed commit of the following: commit 8a62a78 Author: Lean Mendoza <[email protected]> Date: Wed May 10 16:03:27 2023 -0300 update @dcl/ts-proto commit c3bd264 Merge: 2ebeae0 dce1b9e Author: Lean Mendoza <[email protected]> Date: Tue May 9 08:55:46 2023 -0300 Merge remote-tracking branch 'origin/refactor/sdk6-component-serialization' into refactor/sdk6-component-serialization commit dce1b9e Author: Leandro Mendoza <[email protected]> Date: Wed May 3 12:06:34 2023 -0300 change Color4 for ecs6 commit 8d58b02 Merge: 438eb7d 286ee57 Author: Lean Mendoza <[email protected]> Date: Wed May 3 08:59:15 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit 438eb7d Author: Leandro Mendoza <[email protected]> Date: Wed May 3 08:53:02 2023 -0300 fix albedoColor4 commit 84aa511 Author: Leandro Mendoza <[email protected]> Date: Mon May 1 13:59:05 2023 -0300 fix makefile commit c8491e5 Merge: 3f5df94 8b14b40 Author: Lean Mendoza <[email protected]> Date: Mon May 1 12:55:42 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit 3f5df94 Author: Leandro Mendoza <[email protected]> Date: Mon May 1 12:42:59 2023 -0300 fix ts-proto library commit b36a879 Author: Leandro Mendoza <[email protected]> Date: Thu Apr 27 10:09:00 2023 -0300 workaround commit fdbfdd2 Author: Leandro Mendoza <[email protected]> Date: Thu Apr 27 10:06:00 2023 -0300 add missing components (interface) commit 228e128 Author: Leandro Mendoza <[email protected]> Date: Thu Apr 27 09:54:26 2023 -0300 add missing components commit a365920 Merge: c734932 8494b7b Author: Lean Mendoza <[email protected]> Date: Wed Apr 26 11:30:25 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit c734932 Author: Leandro Mendoza <[email protected]> Date: Wed Apr 26 09:45:33 2023 -0300 fix ui input text commit 9a9733f Merge: ea1a486 32cf19f Author: Lean Mendoza <[email protected]> Date: Mon Apr 24 10:56:39 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit ea1a486 Author: Lean Mendoza <[email protected]> Date: Mon Apr 24 10:55:01 2023 -0300 missing properties commit a699a41 Author: Lean Mendoza <[email protected]> Date: Mon Apr 24 09:48:09 2023 -0300 missing properties commit 2e9e708 Merge: fa08d1b 3dc0529 Author: Lean Mendoza <[email protected]> Date: Mon Apr 24 09:27:23 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit fa08d1b Author: Leandro Mendoza <[email protected]> Date: Wed Apr 19 21:52:39 2023 -0300 fix uivalues commit 6e9b6d4 Author: Leandro Mendoza <[email protected]> Date: Tue Apr 18 16:41:16 2023 -0300 add ray query commit 32fb363 Merge: 7cd2ae7 c48ea0a Author: Lean Mendoza <[email protected]> Date: Tue Apr 18 12:05:36 2023 -0300 Merge branch 'main' into refactor/sdk6-component-serialization commit 7cd2ae7 Author: Leandro Mendoza <[email protected]> Date: Tue Apr 18 09:25:08 2023 -0300 make all optional commit efcdb98 Author: Leandro Mendoza <[email protected]> Date: Mon Apr 17 14:32:40 2023 -0300 change to oneof commit 2bf3784 Author: Leandro Mendoza <[email protected]> Date: Mon Apr 17 10:41:55 2023 -0300 change file names commit 4e12942 Author: Leandro Mendoza <[email protected]> Date: Mon Apr 17 09:45:08 2023 -0300 refactor: move engine_interface and add components data
This reverts commit 4b98e1a. Co-authored-by: jmoguilevsky <[email protected]>
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.
Fantastic work 🎉 Having these messages clears the way for very interesting features!
@@ -14,9 +14,45 @@ message Packet { | |||
Chat chat = 5; | |||
Scene scene = 6; | |||
Voice voice = 7; | |||
SdkComponent sdk_component = 8; | |||
CustomSignal signal = 9; |
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.
I may have gone overboard with this comment. Sorry! 😅
I recommend we change the names to Component
and Custom
.
My thoughts:
Rationale for Component
-
I think we should avoid having hard references to the SDK at the protocol layer. It's fine for comments, but a variable name feels like conceptually breaking through the layers 👊 .
-
I don't think the word
Component
needs clarification. The meaning is very established in our terminology, and has been for a long while. -
While
Sdk
does additionally imply that theComponent
is standard, that's a reasonable assumption of any message in the protocol unless otherwise specified.
Rationale for Custom
-
The term
Signal
is associated with semantics that don't quite fit here. -
Custom
, removingSignal
, matches the style of the other names (e.g. we don't haveChatMessage
orVoiceSample
) and is explicit about the purpose of the field: this is for unspecified stuff. -
I can't think of other places where we'd reuse this concept of
Signal
. If this is going to be the only case, I don't think we should introduce it.
// custom implementation-dependant signals. | ||
// | ||
// It is recommended that signal names are namespaced to avoid collisions. For | ||
// example, a signal to request a profile could be named "decentraland.profile-request.v1" |
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 recommendation is super reasonable. Would it be a good idea to enforce it by having a namespace
field?
Edit or maybe specify that there must be at least 1 standardized separator (.
, /
, whatever) in the string
message SdkComponent { | ||
uint32 component_id = 1; | ||
bytes data = 2; | ||
uint32 timestamp = 3; |
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.
It would be nice to add some additional description for the timestamp
field, as with the other two. It's unclear in this context (Components sent over comms) whether this is a Lamport timestamp, a Unix timestamp, or some other time-like value. The same name is used with a float
type for milliseconds in Position
, and there's a nice comment there 😄
option (common.ecs_component_id) = 1087; | ||
|
||
// AvatarCustomizations sets all modifiers over the avatar's apparence. This | ||
message PBAvatarCustomization { |
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.
The word Customization
is a little long. How about Mods
, for modifiers? I'm not sure, nothing better is coming to mind right now.
TF_EASE_IN_OUT_ELASTIC = 28; | ||
TF_EASE_IN_BOUNCE = 29; | ||
TF_EASE_IN_OUT_BOUNCE = 30; | ||
TF_EASE_OUT_BOUNCE = 31; |
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.
I can't wait to see a demo scene full of things animating with weird accelerations 🎉
some of the features were tackled here |
No description provided.