save changes #900

rahulkgupta · 2024-10-24T18:39:21Z

one interesting thing here is that screenshots are only set on the first time a scene is saved. won't this cause drift with the screenshot and what the user has created?

EDIT: actually, this relates to "set scene thumbnail". I dont think we need to ask the user to do this and instead we should just do this for the users ourselves.

rahulkgupta

I think there is more cleanup but this is a good enough start. should deal w/ the save as issue

rahulkgupta · 2024-10-24T18:40:39Z

index.html

@@ -31,6 +31,7 @@
 </head>

 <body>
+  <img id="screenshot-img" src="ui_assets/3DStreet-Viewer-Start-Editor.svg" alt="Invisible Image" style="display:none;">


created an invisible image so that the screentock functions can be synchronous

rahulkgupta · 2024-10-24T18:40:51Z

src/components/screentock.js

@@ -21,7 +21,7 @@ AFRAME.registerComponent('screentock', {
    type: { type: 'string', default: 'jpg' }, // png, jpg, img
    imgElementSelector: { type: 'selector' }
  },


removed all the async code from here

rahulkgupta · 2024-10-24T18:41:31Z

src/editor/api/scene.js

@@ -189,6 +191,102 @@ const checkIfImagePathIsEmpty = async (sceneId) => {
  }
 };

+const saveScreenshot = async (value) => {


moved screenshot functionality to the scene api file so that the toolbar component didn't call functions from the screenshotmodalcomponent

rahulkgupta · 2024-10-24T18:47:41Z

src/editor/api/scene.js

+
+const uploadThumbnailImage = async () => {
+  try {
+    // saveScreenshot('img');


note, i commented this out, since in every call makeScreenshot is called prior

rahulkgupta · 2024-10-24T18:48:04Z

src/editor/components/scenegraph/Toolbar.js

@@ -30,7 +30,6 @@ export default class Toolbar extends Component {
    this.state = {
      // isPlaying: false,
      isSaveActionActive: false,
-      isCapturingScreen: false,


removed all state related functionality for this

rahulkgupta · 2024-10-24T18:50:56Z

src/editor/components/scenegraph/Toolbar.js

-  //   AFRAME.INSPECTOR.close();
-  // }
+  makeScreenshot = () => {
+    const imgHTML = '<img id="screentock-destination">';


use the invisible imgHTML

rahulkgupta · 2024-10-24T18:51:13Z

src/editor/components/scenegraph/Toolbar.js

@@ -343,8 +323,8 @@ export default class Toolbar extends Component {
  };

  toggleSaveActionState = () => {
+    this.makeScreenshot();


explicitly call makescreenshot rather than state management

rahulkgupta · 2024-10-24T18:51:22Z

src/editor/components/scenegraph/Toolbar.js

-                ...prevState,
-                isCapturingScreen: true
-              }));
+              this.makeScreenshot();


call makescreenshot instead of using state

rahulkgupta · 2024-10-25T22:08:40Z

src/editor/api/scene.js

@@ -44,24 +62,16 @@ const deleteScene = async (sceneId) => {
  }
 };

-const updateScene = async (sceneId, userUID, sceneData, title, version) => {


no need to pass user id in updating a scene

rahulkgupta · 2024-10-25T22:11:44Z

src/editor/api/scene.js

@@ -34,6 +36,22 @@ const generateSceneId = async (authorId) => {
  return newSceneId;
 };

+const createScene = async (authorId, sceneData, title, version) => {


added a createScene

rahulkgupta · 2024-10-25T22:11:56Z

src/editor/api/scene.js

@@ -88,28 +96,6 @@ const updateSceneIdAndTitle = async (sceneId, title) => {
  }
 };

-const isSceneAuthor = async ({ sceneId, authorId }) => {


no longer needed

rahulkgupta · 2024-10-25T22:12:21Z

src/editor/components/modals/ScenesModal/ScenesModal.component.jsx

      AFRAME.scenes[0].setAttribute('metadata', 'sceneId', sceneId);
      AFRAME.scenes[0].setAttribute('metadata', 'sceneTitle', sceneTitle);
-
+      AFRAME.scenes[0].setAttribute('metadata', 'authorId', sceneData.author);


when a new scene is loaded, set the authorId

rahulkgupta · 2024-10-25T22:12:40Z

src/editor/components/modals/ScreenshotModal/ScreenshotModal.component.jsx

 // import { loginHandler } from '../SignInModal';

-export const uploadThumbnailImage = async (uploadedFirstTime) => {


move this to a shared location so that Toolbar isn't calling this component

rahulkgupta · 2024-10-25T22:16:49Z

src/editor/components/scenegraph/Toolbar.js

@@ -218,48 +183,42 @@ export default class Toolbar extends Component {
        console.log(
          'no urlSceneId or doSaveAs is true, therefore generate new one'
        );
-        currentSceneId = await generateSceneId(this.props.currentUser.uid);
+        currentSceneId = await createScene(


instead of generating a scene id and then updating a scene, we are simply creating a scene w/ all the relevant info

rahulkgupta · 2024-10-25T22:17:47Z

src/editor/components/scenegraph/Toolbar.js

        console.log('newly generated currentSceneId', currentSceneId);
-        window.location.hash = `#/scenes/${currentSceneId}.json`;


we are doing this later anyways, so this is duplicative

rahulkgupta · 2024-10-25T22:18:15Z

src/editor/components/scenegraph/Toolbar.js

-        await uploadThumbnailImage(true);
-      }
+      AFRAME.scenes[0].setAttribute('metadata', 'sceneId' + currentSceneId);
+      AFRAME.scenes[0].setAttribute(


also updating the authorid now

rahulkgupta · 2024-10-25T22:18:25Z

src/editor/components/scenegraph/Toolbar.js

      // make sure to update sceneId with new one in metadata component!
-      AFRAME.scenes[0].setAttribute('metadata', 'sceneId: ' + currentSceneId);


note, i change sceneId: to sceneId

rahulkgupta · 2024-10-25T22:19:37Z

src/editor/components/scenegraph/ToolbarWrapper.js

-  }, [currentUser, currentSceneId]);
-
-  return <Toolbar currentUser={currentUser} isAuthor={isAuthor} />;
+  return <Toolbar currentUser={currentUser} />;


cleaned this up. no longer passing stuff that doesnt make sense

kfarr · 2024-10-28T20:32:22Z

meta comments:

Before this PR, the state was duplicated across both React and A-Frame components with with Kieran and Alex's "spaghetti code." After this PR state is still duplicated, but there is a lot less spaghetti code and some things are much simpler such as handling of author. Active scene ID, title, author, etc. is probably a candidate for zustand or similar state management system when the time is right.

testing results:

I have a hard time testing the core issue of the "race conditions" of async preview generation not compatible

Instead, I have tested the following to see what is the outcome for preview image generation and uploading:

⭐ press save button when not logged in (then logging in with either google or msft) -- results in uploading black box for thumbnail (if no prior screenshots taken) OR uploading the previously captured image (if successfully saved a capture earlier in the same session) [1] this is confusing as it could be a thumbnail from a scene that is not currently loaded. (ie a black box would be better than the incorrect thumbnail).
when logged in, open another user's scene, then press "save as" from another user's scene -- WORKS as expected
⭐ when logged in, open another user's scene, then move an item, then change the title in the title popup from another user's scene -- RESULT in no thumbnail [2] (different than black, this is not uploading any thumbnail)
when logged in, open my own scene, then press "save as" from my own scene -- WORKS as expected
⭐ when logged in, open my own scene, then click share, then click "Set as scene thumbnail" Now I no longer see a success message after uploading (unless I look at console log) and I may think that something has gone wrong. I do see an unauthorized error if it is raised, which is expected behavior (if I attempt to change the thumbnail of a scene that is not my own)

screenshots:
[1] see left thumbnail

[2] see left thumbnail

kfarr · 2024-10-28T23:37:45Z

all looks great!

change noted as discussed: opening another user's scene, and moving an object, it no longer asks for save as

Rahul Gupta added 2 commits October 24, 2024 11:38

screenshot changes

94bd9c3

cleanup even more

5d13edd

rahulkgupta commented Oct 24, 2024

View reviewed changes

Rahul Gupta added 4 commits October 25, 2024 10:59

more work

6d19dae

cleanup

8ba38f0

more

a282f2c

console cleanup

0d332dc

rahulkgupta changed the title ~~screenshot changes~~ save changes Oct 25, 2024

remove getScene

a3f0cef

rahulkgupta commented Oct 25, 2024

View reviewed changes

fix setAttribute typo

ae667e0

Rahul Gupta added 2 commits October 28, 2024 14:16

respond to comments

e95aa5c

cleanup

5b9f306

kfarr merged commit 6fbafdb into main Oct 28, 2024
1 check passed

kfarr deleted the cleanup-image-upload branch October 28, 2024 23:37

kfarr mentioned this pull request Oct 28, 2024

Do not Save new file if I click Cancel on "Save as" #893

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

save changes #900

save changes #900

rahulkgupta commented Oct 24, 2024 •

edited

Loading

rahulkgupta left a comment

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 24, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

rahulkgupta Oct 25, 2024

kfarr commented Oct 28, 2024 •

edited

Loading

kfarr commented Oct 28, 2024

		// import { loginHandler } from '../SignInModal';

		export const uploadThumbnailImage = async (uploadedFirstTime) => {

		console.log('newly generated currentSceneId', currentSceneId);
		window.location.hash = `#/scenes/${currentSceneId}.json`;

		// make sure to update sceneId with new one in metadata component!
		AFRAME.scenes[0].setAttribute('metadata', 'sceneId: ' + currentSceneId);

save changes #900

save changes #900

Conversation

rahulkgupta commented Oct 24, 2024 • edited Loading

rahulkgupta left a 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

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

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

kfarr commented Oct 28, 2024 • edited Loading

meta comments:

testing results:

kfarr commented Oct 28, 2024

rahulkgupta commented Oct 24, 2024 •

edited

Loading

kfarr commented Oct 28, 2024 •

edited

Loading