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.

Sign up for GitHub

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

Jump to bottom

Improve the UX of ViewportTexture in the editor #64388

Merged
merged 1 commit into from
May 10, 2023
Merged

Improve the UX of ViewportTexture in the editor #64388

merged 1 commit into from
May 10, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Aug 14, 2022 •
edited
Loading

The associated ViewportTextures will update the viewport_path in time when the Viewport's nodepath is changed (caused by renaming the nodes or moving in the SceneTree dock).

0

If the target Viewport is changed by resetting the viewport_path, the ViewportTextures will be re-setup and emit changed signal in time.

1

@Rindbee Rindbee requested a review from a team as a code owner August 14, 2022 12:44
@Calinou Calinou added this to the 4.0 milestone Aug 14, 2022
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch 2 times, most recently from 2136bfb to ad97624 Compare August 15, 2022 00:16
@Rindbee Rindbee mentioned this pull request Aug 17, 2022
Open
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch 2 times, most recently from c47bdb5 to c7a4a5f Compare August 18, 2022 01:09
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from c7a4a5f to 35dd671 Compare October 8, 2022 10:01
clayjohn
clayjohn reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but really needs a review from @reduz

@Rindbee Rindbee mentioned this pull request Nov 8, 2022
Open
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 35dd671 to 9f1afed Compare November 8, 2022 07:19
@clayjohn clayjohn requested a review from reduz December 1, 2022 20:27
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 9f1afed to dd65b8f Compare February 5, 2023 06:34
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga akien-mga mentioned this pull request Feb 23, 2023
Open
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from dd65b8f to 1b0bf21 Compare February 24, 2023 02:18
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 1b0bf21 to 5ff3ddf Compare March 3, 2023 12:26
Chaosus
Chaosus approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

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

Looks fine for me too (fixes the errors with sub-viewports in my project, eliminating the need of setting the texture in the code manually to prevent this).

@KoBeWi
Copy link
Member

KoBeWi commented Apr 27, 2023

I recently opened a very similar PR #75751 🤔

@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 28, 2023 •
edited
Loading

  1. The logic of setup_local_to_scene() in Prevent errors when using ViewportTexture #75751 is better than this, I prefer to have an explicit time rather than a delay.
  2. This PR will update the viewport_path when the target Viewport is moved in the node tree in the editor.
  3. Neither of these two PRs solves the issue that when viewport_path is modified to point to another Viewport, it is not redrawn correctly.
    0

@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 5ff3ddf to 0073445 Compare April 28, 2023 02:36
@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 28, 2023

In editor Play
1 2

test.zip

@KoBeWi
Copy link
Member

KoBeWi commented Apr 29, 2023

Sounds like we could merge #75751 first and then you can rebase to add your changes.
Unless they are incompatible (i.e. you'd to change need my logic), then maybe you can incorporate my PR here and tweak it.

@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 0073445 to 148398a Compare April 29, 2023 13:42
@Rindbee Rindbee requested a review from a team as a code owner April 29, 2023 13:42
@Rindbee
Copy link
Contributor Author

Rindbee commented Apr 29, 2023

OK, now it rebases on #75751.

@KoBeWi
Copy link
Member

KoBeWi commented May 8, 2023

#75751 was merged, so this can be rebased.

@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 148398a to be00ae9 Compare May 8, 2023 14:52
@Rindbee
Copy link
Contributor Author

Rindbee commented May 8, 2023

OK, rebase has been done.

@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from be00ae9 to 698aa75 Compare May 8, 2023 16:21
@Rindbee Rindbee changed the title Improve ViewportTexture Improve the UX of ViewportTexture in the editor May 8, 2023
@akien-mga akien-mga requested a review from KoBeWi May 9, 2023 08:40
KoBeWi
KoBeWi reviewed May 9, 2023
View reviewed changes
scene/main/viewport.cpp Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch 2 times, most recently from cae7c3d to 22f2289 Compare May 10, 2023 00:06
@Rindbee
Improve the UX of ViewportTexture in the editor
af58f1e 
The associated `ViewportTexture`s will update the `viewport_path`
in time when the `Viewport`'s nodepath is changed (caused by renaming
the node names or moving in the SceneTree dock).

If the target `Viewport` is changed by resetting the `viewport_path`,
the `ViewportTexture`s will be re-setup and emit `changed` signal in
time.
@Rindbee Rindbee force-pushed the improve-ViewportTexture branch from 22f2289 to af58f1e Compare May 10, 2023 01:16
@akien-mga akien-mga merged commit 8e608e9 into godotengine:master May 10, 2023
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the improve-ViewportTexture branch May 10, 2023 13:08
@akien-mga
Copy link
Member

Cherry-picked for 4.0.3.

@Rindbee Rindbee mentioned this pull request May 18, 2023
Closed
@YuriSizov YuriSizov mentioned this pull request May 18, 2023
Merged
@YuriSizov
Copy link
Contributor

Cherry-pick reverted, this is no longer planned for 4.0.3. This PR contains some issues (#77207), so it makes sense to give it a bit more time to be tested in master and 4.1 dev builds.

Can be picked again with #77209 and any other additional fixes required.

@Rindbee Rindbee mentioned this pull request May 18, 2023
Closed
@YuriSizov
Copy link
Contributor

We can consider this for 4.0.x again after the 4.1 release, as that would serve as additional testing for this and #77209. At that point I would ask to create a dedicated 4.0 PR based on the master version of the fixes to avoid cherry-picking conflicts.

So for that reason I'm removing the cherry-pick label.

@YuriSizov YuriSizov mentioned this pull request Jun 16, 2023
Merged
@Rindbee Rindbee mentioned this pull request Nov 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clayjohn clayjohn clayjohn left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

@Chaosus Chaosus Chaosus approved these changes

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
bug topic:core topic:editor
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
7 participants
@Rindbee @KoBeWi @akien-mga @YuriSizov @Chaosus @clayjohn @Calinou