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

Title and logo in screenshot #409

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

Algorush
Copy link
Collaborator

I've done everything so far as part of the screentock component. Maybe it's better to rename component to screenshot now? Since there is no need to use the tock method here, as I believe. But maybe there's something I don't know?

- change tock event function to update
- add render call before screenshot
- add toggleHelpers function
- move takeScreenshotNow function to component
Copy link

netlify bot commented Nov 12, 2023

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit 5f3d267
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/6551308b26f4ca00081dce58
😎 Deploy Preview https://deploy-preview-409--3dstreet-core-builds.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kfarr kfarr changed the base branch from main to creator-usability-epic-v1 November 14, 2023 00:31
@kfarr kfarr mentioned this pull request Nov 14, 2023
@kfarr
Copy link
Collaborator

kfarr commented Nov 14, 2023

Hi @Algorush great work so far! I'm combining the "hide helpers" ticket feedback here too.

On the PR related to that ticket that I closed (since its commit is also in this PR) you wrote:

I changed the code of the screentock component a little. I moved the tock event code to the update event, I think it's more efficient. Now this code is not called at every next toсk of the entire scene. But now its not a screentock component...
Before saving the screenshot, I run renderer.render() so that the scene is rendered first.
@kfarr Let me know if its ok or no.

Yes it's ok, seems to work great!

We can also leave the screentock code as it was.
And rename the resulting component to screenshot??
[...]
I've done everything so far as part of the screentock component. Maybe it's better to rename component to screenshot now? Since there is no need to use the tock method here, as I believe. But maybe there's something I don't know?

I don't care what the component name is, but it can't be "screenshot" as that's already reserved for an a-frame component used by the system; might as well keep screentock for now unless there is a need to rename.

And also for the screenshot, perhaps it’s worth hiding the round teleport control pointer?

Yes, I agree that should be hidden just like the other helper objects.

For the logo
(from PR 3DStreet/3dstreet-editor#343) Add id to logo elment to use 3DStreet logo SVG for screenshot

Thanks, I just merged that and later this week I will do a release for the current version of the Editor repo master branch.

@kfarr
Copy link
Collaborator

kfarr commented Nov 15, 2023

From @Algorush

Thank you, so another question is: should the logo in the screenshot consist of "3DStreet Viewer/Editor" text? or only "3DStreet"?

It should just only say "3DStreet"

@Algorush
Copy link
Collaborator Author

Algorush commented Nov 15, 2023

I added the code to get the logo for the screenshot from the svg element. I used async/await to convert svg format to image.
Perhaps it would be easier not to remove the logo element img.viewer-logo-img that appears when the page is first loaded, but hide it. And use the logo from there. Take a look, @kfarr, if this code suits you.

this.el.setAttribute('screentock', 'takeScreenshot', false);
takeScreenshotNow(this.data.filename, this.data.type, this.data.imgElementSelector);
}
this.data.takeScreenshot = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Algorush was there any specific reason to change this
this.el.setAttribute('screentock', 'takeScreenshot', false);
into this
this.data.takeScreenshot = false;

In general we're not supposed to do that with a-frame, always using setAttribute so that lifecycle methods / events are properly triggered, but maybe you needed to in this case for some reason?

Copy link
Collaborator Author

@Algorush Algorush Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

through setAttribute the update event is triggered once again. In this case it is not necessary, in other cases it is different. Here takeScreenshot is used only inside the component. I think so, maybe I misunderstand something?

@kfarr
Copy link
Collaborator

kfarr commented Nov 24, 2023

Ok in testing this here is one final issue I see:

  • the 3dstreet logo does not appear in editor mode, although the street title does appear -- let's wait to fix this if that's ok, since there are a lot of unmerged changes in progress on 3dstreet-editor repo

@kfarr Do you mean not to do PR with corrections in 3DStreet-editor yet? Wait a while?

@kfarr kfarr merged commit 33b1371 into creator-usability-epic-v1 Nov 24, 2023
2 checks passed
@kfarr kfarr deleted the title-and-logo-in-screenshot branch November 24, 2023 23:49
@kfarr kfarr restored the title-and-logo-in-screenshot branch November 24, 2023 23:51
@kfarr
Copy link
Collaborator

kfarr commented Nov 25, 2023

@Algorush yes please do make a correction if possible to the 3dstreet-editor repo through a new PR to master branch.

@Algorush
Copy link
Collaborator Author

Algorush commented Nov 25, 2023

Now with new release of 3DStreet-editor logo is showing in Editor mode

@Algorush yes please do make a correction if possible to the 3dstreet-editor repo through a new PR to master branch.

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.

2 participants