-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add a dialog to customize run instances #65753
Conversation
a7366d0
to
755ea05
Compare
Ok I implemented everything as originally requested. |
0e8029a
to
4e56a84
Compare
I think this looks good, but don't we have this for the multiplayer testing already? |
4e56a84
to
950968d
Compare
488b8d3
to
43139a6
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.
Tested locally (rebased on top of master
da81ca6), it works as expected.
There are some things that should be improved before merging:
- The SpinBox that sets the number of instances should be disabled if Enable Multiple Instances is unchecked.
- Override Main Run Args should have a tooltip to denote that when unchecked, it will append to Main Run Args instead of overridding.
- The dialog can be made extremely tall if set to
20
instances. I'd suggest setting the upper limit to12
, which fits on a 1080p display at 100% scaling.
Additional thought for a future PR:
- If you reduce the number of instances and increase it, options for instances that were removed are lost, even if you do this without closing and reopening the dialog. I suggest keeping removed instance options in memory at least until the dialog is closed (preferably until the editor is closed). This way, you can decrease/increase the number of instances without losing options for unused instances.
- This makes me wonder if multi-instance options should be persisted to
project.godot
as well, just like Main Run Args.
- This makes me wonder if multi-instance options should be persisted to
Main Run Args aren't only for debugging. They can be relevant for an exported project.
I could make the arguments persist across restarts by not truncating the saved array. |
43139a6
to
64dc897
Compare
Reading the code, Main Run Args is only used when running a project from the editor using the run button. It's not even used if you run a project from a terminal using an editor build. We currently don't have an equivalent that also works on an exported project, in case you want to override a CLI argument that has no equivalent project setting. (We should strive to add project settings for those instead, as it's more flexible overall.) |
Ah, I don't know then. Moving it to Project Settings is easy, but I'd do it once someone actually requests it. I think |
https://gist.github.com/mrcdk/3c22d91a3b4fb784d5df60667a33cf1e
from Mastadon: |
This is some debugger hack. This PR makes implementing it much easier, because you can pass different arguments to instances. |
@KoBeWi Could you make it for Project Settings? I may change my mind with one project and do different CLI arguments, and then do different setup with another project. I wouldn't want to ruin the previous project just because it were Editor Settings. If that's what I'm understanding? Hoping it's Project Settings. ^_^ |
It's not in editor settings, it's in project metadata, i.e. hidden per-project settings. |
@KoBeWi so this is from a year ago.. Does it really take this long for approvals? What is the main blocking point? |
64dc897
to
5ad42af
Compare
It needs to be reviewed by a maintainer. Some PRs wait longer than others, because contributors are busy with other stuff and they get forgotten. |
@KoBeWi Oh I see. But I try and give gentle reminder and a nice boost of optimism but Calinou shuts it down and says not to post unless you have any new information (in different issue #). I'm new here and you've been super awesome, but is bumping / reminding not allowed? Should we get more reviewers? How do we become a reviewer? Sorry for all the questions. Thank you for taking the time to answer and being friendly. |
Well anyone can review, but only reviews from @godotengine members count towards merging. idk if you can see it:
That's not how it works. Sure, bumping an issue might get it on someone's radar, but most often it just causes pointless notification, because if someone didn't have time to review something before, a random bump will not change that. The discussion on GitHub should be more focused; if you want to bring attention to a PR, you can come to the contributor chat. You can also leave a review as I mentioned. It has similar effect to a random comment, but brings more value. |
Cool, @Calinou just pointed this to me as I was running into similar situations. One thing I'd like to see added here is to not only be able to specify individual arguments for each instance, but also feature tags. Bonus points if we can also start an instance on a remote device. I'd like to start a spectator server on desktop simultaneously with running my app on a Quest. And would there be an ability to save this as presets, so I can switch between different configurations easily? |
@BastiaanOlij would the Multirun Addon be of any use? Please submit an issue and tag me if you can explain there tagging/presets and I'll see if I can add it for ya. |
5ad42af
to
d0fa160
Compare
See #63529 I'll integrate that once this PR is merged.
No idea how this is done, maybe I could check the native run code 🤔 Not sure about argument support though.
I'd leave that for later. |
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.
Tested locally (rebased on top of 10e1114), it works as expected. The rebase has conflicts, but these are trivial to resolve.
Here are 12 simultaneous instances (the maximum you can set) in their full glory:
Needs a rebase. Overall I feel this dialog would still need a UX pass to make it look a bit nicer and with good use of vertical and horizontal space. And some of the other features we discussed above. But that can be done after this one is merged. |
d0fa160
to
6755ad3
Compare
|
6755ad3
to
535d210
Compare
I suggest that we don't instantiate a limit based on the UX. If so, it means that the UX is bad. I think that adding a scrollbar would ease the pain of having more than 12 instances. |
535d210
to
5ab9e50
Compare
Thanks! |
Closes godotengine/godot-proposals#522
Current look:
(EDIT: the screenshots don't match after I updated the one above, but you get the idea xd)