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

Support to customize launching position for every debug instance #67587

Closed
wants to merge 1 commit into from

Conversation

kdada
Copy link
Contributor

@kdada kdada commented Oct 18, 2022

Change run/window_placement/rect_custom_position to an array to support specify position of every debug instance.

Now I can debug multiplayer instances without Run & moving window & moving window & moving window & moving window.

Snapshot:
image

@kdada kdada requested a review from a team as a code owner October 18, 2022 16:08
@kdada kdada force-pushed the customize-positions branch 2 times, most recently from 0e89bfc to 12392b8 Compare October 18, 2022 19:00
@Chaosus Chaosus added this to the 4.0 milestone Oct 19, 2022
@kdada kdada force-pushed the customize-positions branch from 12392b8 to 2691528 Compare October 19, 2022 17:15
@kdada
Copy link
Contributor Author

kdada commented Oct 23, 2022

@bruvzg do you have time to review this pr?

@kdada kdada force-pushed the customize-positions branch from 2691528 to d25686c Compare October 23, 2022 08:31
@kdada
Copy link
Contributor Author

kdada commented Oct 23, 2022

ping @bruvzg

@Calinou
Copy link
Member

Calinou commented Oct 23, 2022

See also godotengine/godot-proposals#3357 and #64913.

@kdada
Copy link
Contributor Author

kdada commented Oct 23, 2022

See also godotengine/godot-proposals#3357 and #64913.

Thanks for your information.

I read the proposal and #64913. #64913 is different from this PR. This PR only controls the positions of windows and do not touch on debug identity.

I also reviewed #64913 and left some comments.

@kdada
Copy link
Contributor Author

kdada commented Oct 24, 2022

@Calinou Could this pr be merged?

@kdada
Copy link
Contributor Author

kdada commented Oct 29, 2022

@clayjohn Could you also take a look of this PR?

@kdada kdada force-pushed the customize-positions branch from d25686c to cf19fb2 Compare October 31, 2022 14:56
@kdada
Copy link
Contributor Author

kdada commented Oct 31, 2022

Fixed conflicts

@clayjohn
Copy link
Member

The code looks fine to me, but I have a feeling this change will break compatibility for projects that are already using this ProjectSetting. @bruvzg @akien-mga Are we okay with breaking compatibility here?

@bruvzg
Copy link
Member

bruvzg commented Oct 31, 2022

It's an editor setting, so won't break actual projects and probably ok.

@Faless
Copy link
Collaborator

Faless commented Oct 31, 2022

I think these kind of settings should go through a proper "Run Configuration" widget (godotengine/godot-proposals#522 , godotengine/godot-proposals#522 (comment)), likely using project metadata instead of editor settings.

@kdada
Copy link
Contributor Author

kdada commented Nov 1, 2022

It seems the implementation of #65753 is more general. Will Window Placement be deprecated in the future?

@Faless
Copy link
Collaborator

Faless commented Nov 1, 2022

Will Window Placement be deprecated in the future?

It makes sense to move it to wherever we store the run configuration

@JanWerder
Copy link

It seems the implementation of #65753 is more general. Will Window Placement be deprecated in the future?

But it doesn't have explicit window placement options, right?
I think it would make most sense to use a combination of the two.
For most scenarios where you want to move the windows of multiple instances, we are talking about a multiplayer game with multiple clients/servers. For that use-case passing parameters in addition to the window placements would be great, because you can specify which instance would be responsible for what role.

@kdada kdada closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants