-
Notifications
You must be signed in to change notification settings - Fork 227
Fix game preview #143
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
base: master
Are you sure you want to change the base?
Fix game preview #143
Conversation
|
It fix this issue too : Facepunch/sbox-issues#9734 |
|
I'm available tomorrow to format and rebase. |
…e wasnt switch to detected session scene tab
…e wasnt switch to detected session scene tab, without comments
…e wasnt switch to detected session scene tab, without comments
2753077 to
91b2dd8
Compare
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.
Pull request overview
This PR fixes a game preview issue (GitHub issue #9749) by improving the state management and focus handling in the scene editor system.
Changes:
- Added early return optimization in
SceneEditorSession.MakeActive()to avoid redundant operations - Added focus management for scene dock when bringing to front
- Updated viewport widget selection logic to prevent unnecessary updates
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| SceneViewportWidget.cs | Modified condition to check if viewport is already selected before updating |
| SceneViewWidget.cs | Added duplicate Current assignment (redundant) and reformatted early returns |
| SceneDock.cs | Removed extra whitespace |
| SceneEditorSession.cs | Added early return optimization and focus call in BringToFront |
| EditorScene.cs | Added early return for non-prefab sessions and MakeActive call before play |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( SceneEditorSession.Active is not PrefabEditorSession ) | ||
| return SceneEditorSession.Active; | ||
|
|
Copilot
AI
Jan 13, 2026
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.
This check is redundant with the check at line 168. If SceneEditorSession.Active is not a PrefabEditorSession, then it cannot be a PrefabScene either (since PrefabEditorSession manages PrefabScene). The early return at line 164-165 makes the check at line 168 unnecessary for the same case, creating duplicate logic that reduces code clarity.
| if ( SceneEditorSession.Active is not PrefabEditorSession ) | |
| return SceneEditorSession.Active; |
|
|
||
| var hasMouseFocus = hasMouseInput; | ||
| if ( IsFocused && SceneViewWidget.Current.IsValid() ) | ||
| if ( IsFocused && SceneViewWidget.Current.LastSelectedViewportWidget != this && SceneViewWidget.Current.IsValid() ) |
Copilot
AI
Jan 13, 2026
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.
The condition checks are in the wrong order. The code checks SceneViewWidget.Current.LastSelectedViewportWidget before verifying SceneViewWidget.Current.IsValid(). If SceneViewWidget.Current is null or invalid, accessing .LastSelectedViewportWidget will throw a null reference exception. The IsValid() check should be performed first before accessing any properties of SceneViewWidget.Current.
| if ( IsFocused && SceneViewWidget.Current.LastSelectedViewportWidget != this && SceneViewWidget.Current.IsValid() ) | |
| if ( IsFocused && SceneViewWidget.Current.IsValid() && SceneViewWidget.Current.LastSelectedViewportWidget != this ) |
|
|
||
|
|
Copilot
AI
Jan 13, 2026
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.
Extra blank line added. This creates inconsistent whitespace formatting with the rest of the codebase.
|
|
||
| if ( isActive ) | ||
| { | ||
| // Log.Info("SceneViewWidget#Frame.L115 isActive"); |
Copilot
AI
Jan 13, 2026
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.
Commented-out debug code should be removed before merging. If this logging is needed for future debugging, consider using a proper logging level or conditional compilation flag instead of leaving commented code in the codebase.
| // Log.Info("SceneViewWidget#Frame.L115 isActive"); |
| if ( isActive ) | ||
| { | ||
| // Log.Info("SceneViewWidget#Frame.L115 isActive"); | ||
| Current = this; | ||
| } | ||
|
|
Copilot
AI
Jan 13, 2026
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.
The assignment Current = this is duplicated. This line sets Current = this when isActive is true, but the same assignment already occurs at line 108 under the same condition. The second assignment at line 127 is redundant and should be removed.
| if ( isActive ) | |
| { | |
| // Log.Info("SceneViewWidget#Frame.L115 isActive"); | |
| Current = this; | |
| } |
Hello there is three commit, if you want to test, take first commit i added comments usefull as you can see callstacks.
The PR fix this issue : github.com/Facepunch/sbox-issues/issues/9749
debug-fix-game-preview.log