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

Headless - Improve group transfer and add API #9874

Merged
merged 8 commits into from
May 29, 2024

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Mar 24, 2024

When merged this pull request will:

  • Checks for valid HCs before trying to transfer groups.
  • Adds API for group transferral: 1 targeted event before a group is transferred, 1 targeted event after (targets are the current client and the HC it's going to move to).
    The API is very much WIP and I'd appreciate any input @LinkIsGrim.
    Because of it being WIP, I haven't made any documentation for it yet.

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

@johnb432 johnb432 added the kind/enhancement Release Notes: **IMPROVED:** label Mar 24, 2024
@jonpas
Copy link
Member

jonpas commented Mar 24, 2024

Is global event really a good idea here? That could be a lot of traffic on mission start.

@johnb432
Copy link
Contributor Author

Is global event really a good idea here? That could be a lot of traffic on mission start.

I'm really not sure. I imagine a targeted event should suffice tbh.

@LinkIsGrim
Copy link
Contributor

Functions to black/white-list a unit/group/array of units and module/Zeus interactions to go with them? I can add context menu on ZEN side afterwards.

@jonpas
Copy link
Member

jonpas commented Mar 25, 2024

Black/white-list is just GROUP/UNIT setVariable ["acex_headless_blacklist", true/false, true];. I would not mind ZEN modules for this though, as well as "move to Server/HC1/HC2/...". :)

@johnb432
Copy link
Contributor Author

Black/white-list is just GROUP/UNIT setVariable ["acex_headless_blacklist", true/false, true];. I would not mind ZEN modules for this though, as well as "move to Server/HC1/HC2/...". :)

GROUP/UNIT setVariable ["acex_headless_blacklist", true/false]; is only set locally though, not synced across the network. Might be safer to include a function that can be called via event.

For the modules, should it be 1 module, in which we have a menu that can select where the AI should be moved to or should there be 4 separate modules?

@jonpas
Copy link
Member

jonpas commented Mar 25, 2024

is only set locally though, not synced across the network. Might be safer to include a function that can be called via event.

Add a 3rd argument (true) like I did and it's global. And it should only ever be applied globally anyways.

I am not the best person to ask for UX, but those should live in ZEN anyways so maybe it's better to have a discussion there.

@johnb432
Copy link
Contributor Author

johnb432 commented Mar 25, 2024

is only set locally though, not synced across the network. Might be safer to include a function that can be called via event.

Add a 3rd argument (true) like I did and it's global. And it should only ever be applied globally anyways.

I'm fully aware, it's just that the code never did it, making it inconsistent if we do start syncing it in this case as opposed to not syncing it in

_object setVariable [QXGVAR(blacklist), true];

#9873 removes the problem entirely, so it won't matter anymore once that's merged.


I am not the best person to ask for UX, but those should live in ZEN anyways so maybe it's better to have a discussion there.

Ah, I thought we were going to add it in ACE' Zeus component.

I've worked with ZEN modules before, can make them if wanted.

@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Mar 25, 2024

If we don't have modules in ACE Headless (I think they're unnecessary and cumbersome, just mentioned them for sake of completion) we should have Zeus Interactions available, at least.

ZEN integration is just making that available in context menu.

Blacklist variable should always be set globally IMO (thus, the function. I strongly dislike variables as API.). It's one bool per object, it's not going to break networking.

@jonpas
Copy link
Member

jonpas commented Mar 25, 2024

_object setVariable [QXGVAR(blacklist), true];

That's just a buggy remnant, that isKindOf check was just used for UAVs, it should have been public. However it worked because it was set on server and so those objects were never transferred to begin with (it might break with manual transfer technically).

Technically only server cares about blacklist variable, but what @LinkIsGrim said is true. I don't see a difference between function and variable here. If you want a function, it must also transfer back to server if it has been transferred already, as that is what one would expect.

@johnb432
Copy link
Contributor Author

johnb432 commented Mar 25, 2024

That's just a buggy remnant [...]

I knew it worked, but I didn't realise it wasn't intentional. I'm not opposed to setting it public at all.

If you want a function, it must also transfer back to server if it has been transferred already, as that is what one would expect.

I didn't think about that, but on the other hand, I think it might be best to make that optional (on by default though), as in some cases one might want AI to remain local to the client without being transferred back to the server.

Having a function would give us the liberty to add functionality similar to this in the future, so I'll implement it.

I'll also just implement Zeus interactions (no modules). @LinkIsGrim do you want to handle the ZEN context actions? I can do it, I know how, but I'll let you if you want to.

@LinkIsGrim
Copy link
Contributor

I'm already working on other ZEN-ACE stuff, can handle that, just need functions to call

@jonpas
Copy link
Member

jonpas commented Mar 25, 2024

but I didn't realise it wasn't intentional

Maybe it was, ask me 7 years ago. :P

I think it might be best to make that optional (on by default though), as in some cases one might want AI to remain local to the client without being transferred back to the server.

Agreed.

@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Mar 27, 2024

I'd like to be able to do this easily as a user:

  • Blacklist/whitelist selected (no locality transfer)
  • Transfer selection to my client (blacklist if not already blacklisted)
  • Transfer selection to server (same as above)
  • Force rebalance (whitelist if not already whitelisted)

Doesn't even necessarily need HC to be active/enabled (this could be part of ACE Zeus) as long as black/whitelisting happens.

Is implementing this (from a 3rd party perspective) possible with current API?

@johnb432
Copy link
Contributor Author

johnb432 commented Mar 27, 2024

I'd like to be able to do this easily as a user:

* Blacklist/whitelist selected (no locality transfer)

* Transfer selection to my client (blacklist if not already blacklisted)

* Transfer selection to server (same as above)

* Force rebalance (whitelist if not already whitelisted)

Doesn't even necessarily need HC to be active/enabled (this could be part of ACE Zeus) as long as black/whitelisting happens.

Is implementing this (from a 3rd party perspective) possible with current API?

I've made it more general, so that you can transfer units you are blacklisting to any machine.

* Force rebalance (whitelist if not already whitelisted)

By that I'm guessing you are whitelisting units and you want to have the option, so that they auto-rebalance?

What do you mean by "force rebalance"? Do you want it to trigger a rebalance or do you want it to forcibly rebalance all units over all HCs?

See the below for the source of confusion:

if (!_force && {(owner _x) in [_idHC1, _idHC2, _idHC3]}) exitWith {

@johnb432
Copy link
Contributor Author

Is implementing this (from a 3rd party perspective) possible with current API?

Yes, although I want you to check it, just to be sure.

@johnb432 johnb432 added this to the 3.18.0 milestone Apr 14, 2024
@LinkIsGrim LinkIsGrim merged commit 1205895 into master May 29, 2024
5 checks passed
@LinkIsGrim LinkIsGrim deleted the headless-transfer-groups branch May 29, 2024 18:50
blake8090 pushed a commit to blake8090/ACE3 that referenced this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants