Dylan/onboarding-upgrades #28

dylanh724 · 2024-02-07T06:49:24Z

Problem

Onboarding towards the path of C# and Unity experiences a minor block, alongside minor inconsistencies, bloat and room for UX improvements.

Solution

Fixed a blocking syntax error with the "Hello World" CLI example.
Fixed broken .unitypackage URL in C# ref.
Polished with minor typo fixes, code consistency (basing on other examples), more internal reference links.
Additional warning regarding C# speed vs Rust.
In the Unity tutorial, we go the path of Rust for the module: Unity devs are more-likely wanting C# for a quickstart (since we 100% know they know C#, but unsure regarding Rust):
- Linked to C# Module tutorial before we jump to Rust, as a placeholder.
- For pt2 (a future PR), I'll work with @jdetter to parity the Rust code in C# to diverge tutorial a bit (or perhaps have code tabs, as John suggested).
Renamed Unity3d refs to the modern-named Unity and added "or Rider" appended to Visual Studio mentions.
Added OneTimeQuery C# api ref.
Related, I opened another PR on spacetime-web that fixes all external links that are currently showing as unstyled.
Verbose Details with Screenshots (Google Doc - Clockwork Only)

Testing

There was a README note instructing how to test below, but there seemed to be no instructions below (I also removed this note). However, I was able to use the preview pane in Rider to ensure expected visuals.

(Edit: Squashed for clarity)

refactor: Whats next cleanup, +unity, -bloat Removed redundant text while there refactor: Unity quickstart fixes, impr, prettify refactor: Unity pt1 fixes, impr, prettify fix(README): Rm "see test edits below" ref * !exists refactor(minor): General onboarding cleanup * Shorter, prettier, consistent fix(sdks/c#): Broken unitypackage url feat(sdks/c#): Add OneTimeQuery api ref

bfops

Nice, these are largely good improvements 👍

Left a few specific questions/concerns below

docs/unity/part-1.md

docs/sdks/c-sharp/quickstart.md

bfops · 2024-02-07T18:28:40Z

docs/sdks/c-sharp/index.md


-https://sdk.spacetimedb.com/SpacetimeDBUnitySDK.unitypackage
+In Unity navigate to the `Assets > Import Package > Custom Package` menu in the menu bar. Select your `SpacetimeDB.Unity.Comprehensive.Tutorial.unitypackage` file and leave all folders checked.


hm, you're right, but I don't know that this name change was intentional.. @jdetter ?

bfops · 2024-02-07T19:01:48Z

docs/unity/part-1.md

@@ -105,6 +112,8 @@ At this point you should have the single player game working. In your CLI, your
 spacetime start
 ```

+💡Below examples Rust language, [but you may also use C#](../modules/c-sharp/index.md) at the cost of 1/2 the speed. 


I'm not sure that we should advertise/codify this speed difference 🤔 it's kind of in flux. @jdetter do you have thoughts?

On a second pass, there's a chance it could sound like we're passively-aggressively pushing them towards Rust, but at the same time... this would probably explain why a Unity tutorial shows Rust server code instead of C# (that was the question in my head that triggered the speed addition suggestion)? I had no clue the diff was so huge.

For now, I removed the speed memo, but left the C# link. After talking with @jdetter , we'll actually soon diverge this Unity pt1 section into C# as well - not just Rust. There's a draft for this for the future onboarding upgrades (pt2)

docs/unity/part-1.md

bfops · 2024-02-07T19:07:30Z

docs/unity/part-1.md

-        PlayerMovementController.Local.GetModelRotation(),
-        PlayerMovementController.Local.IsMoving());
+   bool hasUpdatedRecently = lastUpdateTime.HasValue && 
+   Time.time - lastUpdateTime.Value > 1.0f / movementUpdateSpeed;


I like breaking up these variables 💯

..but this line break/indent is super confusing 😅

also, you didn't introduce this issue, but since we're here: this code is wrong!

Maybe we can add another variable?

float? deltaTime = Time.time - lastUpdateTime; bool hasUpdatedRecently = deltaTime.HasValue && deltaTime.Value < 1.0f / movementUpdateSpeed; bool isConnected = SpacetimeDBClient.instance.IsConnected(); if (hasUpdatedRecently || !isConnected) { return; }

Got it -- also, I'm wondering how you folks feel about this:

if (hasUpdatedRecently || !isConnected) return;

Do you like the shortcut versions for simple ifs? I didn't touch these, just in case.

I'm indifferent - @jdetter does it matter to you?

bfops · 2024-02-07T19:54:14Z

docs/modules/c-sharp/quickstart.md

 ## Call Reducers

 You can use the CLI (command line interface) to run reducers. The arguments to the reducer are passed in JSON format.

 ```bash
-spacetime call <module-name> send_message '["Hello, World!"]'
+spacetime call <module-name> send_message "Hello, World!"


I assume it caused some cryptic error with the square brackets? 😅

'twas a cryptic err ;)

- PR review change requests - Additionally: hasUpdatedRecently fix and reformatting

dylanh724 · 2024-02-08T01:05:51Z

✅ Went through all the raised issues

dylanh724 · 2024-02-08T04:04:31Z

New commit: Filter and Find being shown interchangeably in C# tutorial. Normalized to Find instead of Filter, since discussing just looking if 1 user exists.

dylanh724 · 2024-02-08T06:06:42Z

Edited in the Google Doc draft with screenshots and verbose details (only Clockwork can view+comment for now):

- Used FindBy since that was what the tutorial used, and also looking for a single Identity. - Note: There may be a similar rust discrepancy in the Unity pt1 tutorial. It'll work with Filter, but just simply less consistent. Holding off on that since my Rust syntax knowledge !exists.

* Duplicate comments found both above and within funcs

dylanh724 · 2024-02-08T07:47:04Z

Additionally pushed copy+paste redundant comments in Unity pt1:

(💡 I'm only pushing very minor changes from here, while this is still pending; let me know if you prefer I stop completely. Anything new is unsquashed.)

* Removed `System.Runtime.CompilerServices` * SpacetimeDB.Module seems to already include this (merged the info)

dylanh724 · 2024-02-08T09:33:43Z

New minor commit: Removed a Unity-pt1 using directive, and merged some info with another using statement that seems to now include the one removed:

dylanh724 · 2024-02-08T10:50:45Z

New minor commit: Better code spacing for a crunched block

dylanh724 · 2024-02-08T11:33:42Z

New minor commit: Add a memo that standalone mode will run in the foreground, which may throw some off when you need to publish (and keep this open). Thanks for the idea, @PuruVJ (I experienced the same friction, myself)

(Final addendum for this shift, and perhaps for this entire PR minus other suggestions)

PuruVJ · 2024-02-08T11:41:05Z

docs/getting-started.md


 ```bash
 spacetime start
 ```

 The server listens on port `3000` by default, customized via `--listen-addr`.

+💡 Standalone mode will run in the foreground: new commands will require a new terminal window.


How about writing in first person: Standalone mode will run in the foreground. Open a new terminal to run new commands

It feels easier to parse

That's a good one, too. In the end, I changed it to "💡 Standalone mode will run in the foreground", since there are numerous ways to do this (someone pointed out you can also add & to make it run in the bg). Perhaps we don't need too much info, as long as they know the gist?

* At general quickstart for `spacetime start`

* Also, removed the "speed" loss mention of C#

dylanh724 · 2024-02-09T02:20:08Z

New minor commit: Removed speed mention, added foreground memo for standalone mode in Unity pt1 (since we're going to issue 2 commands one-after-another).

dylanh724 · 2024-02-09T07:11:35Z

There's another fix I want to add, but didn't yet since it's a bit more complex:

Swapping PlayerComponent to nullable with FindBy instead of FilterBy will fix a syntax err. This part is straight-forward.
Then, we probably want to check if is not null, but this suddenly makes me realize: What if it is null?
- In the tutorial, we never actually used the CreatePlayer Reducer, although we did create it. Is this the place to use if the PlayerComp is null? CC @bfops

Edit: How about this?

Edit 2:

[Internal] Discussion: https://discord.com/channels/931210784011321394/1205409581120102451

Edit 3:

Discussion resolved. Final edit:

bfops · 2024-02-13T22:05:23Z

docs/unity/part-1.md

-        PlayerMovementController.Local.GetModelRotation(),
-        PlayerMovementController.Local.IsMoving());
+   float? deltaTime = Time.time - lastUpdateTime;
+   bool hasUpdatedRecently = deltaTime.HasValue && deltaTime.Value < 1.0f / movementUpdateSpeed;


(review note: I asked to switch this to <, because as far as I can tell the > was a bug)

- After a verbose discussion, we will eventually swap to FindBy for single-result queries, but not in this PR. - For now, the syntax err is fixed by making the var nullable and suffixing a LINQ FirstOrDefault(). Approved by Tyler in Discord. - We never *actually* created a player in the tutorial. This creates the player. Approved by Tyler in Discord.

dylanh724 · 2024-02-15T08:23:57Z

Final edit, following the resolution of the FilterBy vs FindBy discussion:

Both the top line and null handling approved by Tyler in Discord.

dylanh724 · 2024-02-15T08:24:36Z

Just a quick bump ping to @bfops / @jdetter for review when free. No rush (I'm working on other projects while waiting).

bfops

Oops, thanks for the bump - meant to approve this! These improvements look great 👍

But let's not merge until John's had a chance to glance.

bfops

I think this looks good. We synced with @jdetter in a meeting as well.

One last question

docs/unity/part-1.md

dylanh724 · 2024-03-29T09:16:22Z

After being informed there was only 1 remaining question (answered+fixed), merging!

dylanh724 requested review from jdetter and bfops February 7, 2024 06:53

dylanh724 added the documentation Improvements or additions to documentation label Feb 7, 2024

dylanh724 force-pushed the dylan/onboarding-upgrades branch 2 times, most recently from 78e2bc8 to 1b4d40d Compare February 7, 2024 07:00

dylanh724 marked this pull request as draft February 7, 2024 07:05

dylanh724 force-pushed the dylan/onboarding-upgrades branch from 1b4d40d to 60472af Compare February 7, 2024 07:58

dylanh724 marked this pull request as ready for review February 7, 2024 08:00

bfops requested changes Feb 7, 2024

View reviewed changes

dylanh724 added 3 commits February 8, 2024 08:39

doc: Onboarding impr, fixes, consistency, cleanup

61629ad

fix: Rm redundant 'module_bindings' mention

45cc2d2

fix: Floating period, "arbitrary", "important":

571a04b

- PR review change requests - Additionally: hasUpdatedRecently fix and reformatting

dylanh724 force-pushed the dylan/onboarding-upgrades branch from 0ae22ff to f54d3d4 Compare February 8, 2024 04:03

dylanh724 force-pushed the dylan/onboarding-upgrades branch from f54d3d4 to 47b473b Compare February 8, 2024 07:41

fix(Unity-pt1): Rm copy+paste redundant comments

4833cf7

* Duplicate comments found both above and within funcs

fix(unity): Rm unused using statement +merged info

612a680

* Removed `System.Runtime.CompilerServices` * SpacetimeDB.Module seems to already include this (merged the info)

refactor(minor): Code spacing for grouping/clarity

d030309

PuruVJ reviewed Feb 8, 2024

View reviewed changes

feat: 'Standalone mode runs in foreground' memo

4986db6

* At general quickstart for `spacetime start`

dylanh724 force-pushed the dylan/onboarding-upgrades branch from 80156cb to 4986db6 Compare February 9, 2024 01:23

refactor(unity-pt1): Standalone mode foreground memo

c12c0c8

* Also, removed the "speed" loss mention of C#

bfops reviewed Feb 13, 2024

View reviewed changes

bfops approved these changes Feb 15, 2024

View reviewed changes

This was referenced Feb 23, 2024

Dylan/unity-tutorials-csharp-parity #31

Merged

PROPOSAL: Polish the existing content #26

Open

dylanh724 changed the title ~~Dylan/onboarding upgrades (pt1)~~ Dylan/onboarding-upgrades Feb 26, 2024

bfops requested review from bfops and removed request for jdetter March 14, 2024 17:03

bfops self-assigned this Mar 14, 2024

bfops reviewed Mar 14, 2024

View reviewed changes

docs/unity/part-1.md Outdated Show resolved Hide resolved

fix: Remote player is null check removal

58651ab

dylanh724 mentioned this pull request Mar 29, 2024

Dylan/refactor/unity tutorial revamp #37

Closed

dylanh724 merged commit ab6b6b4 into clockworklabs:master Mar 29, 2024

dylanh724 deleted the dylan/onboarding-upgrades branch March 29, 2024 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dylan/onboarding-upgrades #28

Dylan/onboarding-upgrades #28

dylanh724 commented Feb 7, 2024 •

edited

Loading

bfops left a comment

bfops Feb 7, 2024

bfops Feb 7, 2024

dylanh724 Feb 8, 2024 •

edited

Loading

dylanh724 Feb 9, 2024 •

edited

Loading

bfops Feb 7, 2024

bfops Feb 7, 2024

dylanh724 Feb 8, 2024

bfops Feb 13, 2024

bfops Feb 7, 2024

dylanh724 Feb 8, 2024

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024 •

edited

Loading

dylanh724 commented Feb 8, 2024 •

edited

Loading

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024 •

edited

Loading

PuruVJ Feb 8, 2024

dylanh724 Feb 9, 2024

dylanh724 commented Feb 9, 2024

dylanh724 commented Feb 9, 2024 •

edited

Loading

bfops Feb 13, 2024

dylanh724 commented Feb 15, 2024 •

edited

Loading

dylanh724 commented Feb 15, 2024

bfops left a comment

bfops left a comment

dylanh724 commented Mar 29, 2024


		https://sdk.spacetimedb.com/SpacetimeDBUnitySDK.unitypackage
		In Unity navigate to the `Assets > Import Package > Custom Package` menu in the menu bar. Select your `SpacetimeDB.Unity.Comprehensive.Tutorial.unitypackage` file and leave all folders checked.

Dylan/onboarding-upgrades #28

Dylan/onboarding-upgrades #28

Conversation

dylanh724 commented Feb 7, 2024 • edited Loading

Problem

Solution

Testing

bfops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanh724 Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

dylanh724 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024 • edited Loading

dylanh724 commented Feb 8, 2024 • edited Loading

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024

dylanh724 commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanh724 commented Feb 9, 2024

dylanh724 commented Feb 9, 2024 • edited Loading

Edit: How about this?

Edit 2:

Edit 3:

Choose a reason for hiding this comment

dylanh724 commented Feb 15, 2024 • edited Loading

dylanh724 commented Feb 15, 2024

bfops left a comment

Choose a reason for hiding this comment

bfops left a comment

Choose a reason for hiding this comment

dylanh724 commented Mar 29, 2024

dylanh724 commented Feb 7, 2024 •

edited

Loading

dylanh724 Feb 8, 2024 •

edited

Loading

dylanh724 Feb 9, 2024 •

edited

Loading

dylanh724 commented Feb 8, 2024 •

edited

Loading

dylanh724 commented Feb 8, 2024 •

edited

Loading

dylanh724 commented Feb 8, 2024 •

edited

Loading

dylanh724 commented Feb 9, 2024 •

edited

Loading

dylanh724 commented Feb 15, 2024 •

edited

Loading