Skip to content
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

Merged
merged 11 commits into from
Feb 18, 2025

Conversation

AlejandroAlvarezMelucciDCL
Copy link
Collaborator

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL commented Feb 3, 2025

Issue: #2788

QA TEST STEPS

Validate happy path:

  1. Download this test scene and decompress it somewhere
  2. Enter the server folder with a terminal and run npm i and then npm run start, leave the terminal running.
  3. Enter the scene folder with a terminal and run npm i and then npm run start -- -b, leave the terminal running.
  4. Download the build from this PR and connect it to the running scene using the console command according to your OS
  5. The client should open and the scene should be shown
    image
  • The scene has 3 cubes.
  • The 2 blue cubes are clickable (Note: wait a few seconds to get a response before clicking again, there's no queue handling and the clicks can clog the text updates).
  1. Click the blue cube from the left with the legend "Send HTTP request", it sends an HTTP (not secured) request to the server.
  2. A message above the multicolored cube will appear for 5 seconds and then it'll reset to the initial message.
    image
  3. Click the blue cube from the right with the legend "Send WS request", it sends a WebSocket (not secured) to the server.
  4. A message above the multicolored cube will appear for 5 seconds and then it'll reset to the initial message.
    image

Validate errors when the server is NOT running:

  1. Close both terminals and the client
  2. Enter the scene folder with a terminal and run npm i and then npm run start -- -b, leave the terminal running.
  3. Run this PR client again and connect to the running scene using the console command according to your OS
  4. The client should open and the scene should be shown
  5. Click the right cube, this message should be displayed:
    image
  6. Click the left cube, this message should be displayed:
    image

(SEE UPDATE SECTION BELOW, this is deprecated) Validate on issue #2788's scene

Notes:

  • The scene provided in the issue seems to be complex and could have errors not related to http requests.
  • The errors in that scene could be related to the scene code, calypso or something else.
  • If needed, new issues should be created to address those but isolated from this fix for http and ws connections in preview mode

UPDATE

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

  1. Unzip
  2. Open terminal inside the server folder
  3. Run npm i
  4. Run npm run start
  5. Leave the terminal running with the server
  6. Open new terminal in the root folder of the unzipped content
  7. Run npm i
  8. Run npm run start -- -b
  9. Leave the terminal running with the scene
  10. Open this PR's build and connect it to the running scene using the console command according to your OS

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added the force-build Used to trigger a build on draft PR label Feb 3, 2025
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL linked an issue Feb 3, 2025 that may be closed by this pull request
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL added clean-build Used to trigger clean build on PR and removed force-build Used to trigger a build on draft PR labels Feb 6, 2025
Copy link
Member

@pravusjif pravusjif left a 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.

@AlejandroAlvarezMelucciDCL
Copy link
Collaborator Author

AlejandroAlvarezMelucciDCL commented Feb 11, 2025

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
Maybe isPreviewMode, or inPreviewMode it's better?

@pravusjif
Copy link
Member

pravusjif commented Feb 11, 2025

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 Maybe isPreviewMode, or inPreviewMode it's better?

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 isPreviewMode is better. However, that means we should also change all the "local scene development" mentions in all the code, so let's do that renaming change in a different dedicated PR (although I don't agree with that change...).

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...

@pravusjif pravusjif dismissed their stale review February 12, 2025 09:41

I don't agree with the "isPreview" naming, but don't want to block the PR

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL removed the clean-build Used to trigger clean build on PR label Feb 12, 2025
Copy link

@Ludmilafantaniella Ludmilafantaniella left a 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

@Ludmilafantaniella
Copy link

Approved! ✅

Tested on both Windows and Mac, and the scene is working correctly. 🚀

UPDATE-fixed.cube.jumper.scene.mp4

@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL merged commit 68caddc into dev Feb 18, 2025
12 of 13 checks passed
@AlejandroAlvarezMelucciDCL AlejandroAlvarezMelucciDCL deleted the fix/urls-with-http-support-in-preview branch February 18, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URLs with the HTTP protocol are not being accepted
3 participants