-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: 2788 - urls with http support in preview #3281
fix: 2788 - urls with http support in preview #3281
Conversation
…ions in preview mode to the client. Still wip. This commit is to have a build to test
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below. |
…d error handling removing it from js to C#
…updated classes accordingly
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.
isPreview
is not explainatory at all, it's an old name for local scene development, let's rename all the isPreview
with localSceneDevelopment
or similar please.
There were talks to rename and standarize naming local scene development to preview mode, I even created this suggestion: #2861 |
Let's continue that suggestion discussion in that other issue or outside this PR, in any case if we want to change the Local Scene Development concept back to "preview" then yes I think Otherwise, after merging this PR we will have parts of the code referencing the "local scene development" concept, and other parts referencing the "is preview mode" concept, which is an unneeded confusion, since both things are the same... |
I don't agree with the "isPreview" naming, but don't want to block the PR
Explorer/Assets/Scripts/SceneRuntime/Apis/Modules/WebSocket/WebSocketApiWrapper.cs
Outdated
Show resolved
Hide resolved
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.
✅ Approved
Tested on Windows and Mac following the provided steps.
Happy Path: Scene loads correctly, clickable cubes trigger HTTP and WS requests as expected, and messages appear and reset properly.
happy.path-3281.mp4
Error Handling: When the server is not running, the expected error messages are displayed when clicking the cubes.
Validate.errors.when.the.server.is.NOT.running.mp4
Everything is working as intended! 🚀🎉
❗ Note: I couldn't validate issue #2788 because the test scene (cube jumper) is not properly set up for local testing—it requires modifications to connect to a local server. Alejandro is handling this and has reached out to content for validation.
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- ✔️ Log In/Log Out
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
- ✔️ Chat and multiplayer
- ✔️ Profile card
- ✔️ Camera
- ✔️ Skybox
Approved! ✅ Tested on both Windows and Mac, and the scene is working correctly. 🚀 UPDATE-fixed.cube.jumper.scene.mp4 |
Issue: #2788
QA TEST STEPS
Validate happy path:
npm i
and thennpm run start
, leave the terminal running.npm i
and thennpm run start -- -b
, leave the terminal running.Validate errors when the server is NOT running:
npm i
and thennpm run start -- -b
, leave the terminal running.(SEE UPDATE SECTION BELOW, this is deprecated) Validate on issue #2788's scene
Notes:
http
andws
connections in preview modeUPDATE
I did some small changes to the #2788's scene and server to properly use local server as by default is trying to connect to a remote server, here's the zip: fixed cube jumper scene and server.zip
npm i
npm run start
npm i
npm run start -- -b
Our Code Review Standards
https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md