-
Notifications
You must be signed in to change notification settings - Fork 54
Start to backfill some protocol documentation #166
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
Conversation
|
+@SeanCurtis-TRI could I ask for any early feature-review feedback here? I still have some TODOs to fill out, but I should probably check if I'm moving in the right direction. |
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 think this is good. I do somewhat bemoan the informality. There's a part of me wondering if some kind of grammar definition wouldn't be helpful. Especially for the complex set_object(). But this is a definite improvement.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jwnimmer-tri)
|
Thanks. I'll ping you again once I have the additional JSON examples filled in. My hope is that will help mitigate the lack of a more structured specification. |
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 ready for a full review no.
No particular rush, I just want to have this baseline ready for when I start changing the protocol (by allowing url instead of data for assets).
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jwnimmer-tri)
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 ready for a full review now*.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @jwnimmer-tri)
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jwnimmer-tri)
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)
test/texture_text.html line 50 at r3 (raw file):
transparent: true, map: "0c8c99a8-bba8-11ee-b7a2-4b79088b524d", }
BTW If you change the default sided-ness to "2", you can read the text on both sides. The test, as is, is sufficient to show that the text textures work. Extending it slightly to make the text visible from both sides may serve further pedagogical value.
Suggestion:
{
uuid: "6fe7011b-bba7-11ee-b7a2-4b79088b524d",
type: "MeshPhongMaterial",
transparent: true,
map: "0c8c99a8-bba8-11ee-b7a2-4b79088b524d",
// Make the text visible front and back (double sided).
side: 2,
}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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jwnimmer-tri)
See here for a rendered preview.
This change is