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

Add Saved selection for modules and Transfer Ownership Module #501

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

ampersand38
Copy link
Member

@ampersand38 ampersand38 commented Jan 9, 2021

When merged this pull request will:

  • Add global variable zen_editor_savedSelection which tracks the previous-to-last content of curatorSelected, used for selecting entities then dropping a module affecting them.
  • Add Transfer Ownership module using saved selection
  • https://youtu.be/4L-czAJIIcQ
  • https://youtu.be/sAHKlZA-Vz4

@Kexanone Kexanone added this to the 1.10.0 milestone Jan 9, 2021
@Kexanone Kexanone added the feature Adds a new feature label Jan 9, 2021
@Kexanone
Copy link
Member

Kexanone commented Jan 10, 2021

Can you enable edits from maintainers?
Wanted to add the revised selection preview.
Nvm, I was just stupid.

@jonpas
Copy link
Contributor

jonpas commented Jan 11, 2021

Is that naked unit bug fix basically same as in ACEX Headless or did you improve it in any way?

Side-note: It would be nice to have ACEX Headless compatibility when transfering to client (automatically put into headless blacklist for automatic transfers). Or an API event to be able to do it in a mission framework for example.

@Kexanone
Copy link
Member

Kexanone commented Jan 11, 2021

Is that naked unit bug fix basically same as in ACEX Headless or did you improve it in any way?

Oh, I forgot about the ACEX one. I basically improved the Achilles' version of the fix by switching to unscheduled and reducing network traffic by not broadcasting the loadouts to all clients.

Side-note: It would be nice to have ACEX Headless compatibility when transfering to client (automatically put into headless
blacklist for automatic transfers). Or an API event to be able to do it in a mission framework for example.

Yeah, we still have to look into this.

@jonpas
Copy link
Contributor

jonpas commented Jan 11, 2021

I basically improved the Achilles' version of the fix by switching to unscheduled and reducing network traffic by not broadcasting the loadouts to all clients.

ACEX Headless does transmit loadout to everyone as well, and that is for sure an improvement in network traffic. But I don't see how it can be enough of an improvement to "fix" the issue, not while there are a million way more intensive things being broadcasted.

This seems to be a possible better fix (which I haven't implemented in ACEX yet): acemod/ACEX#180 (comment) (also ref. linked BI ticket).

We also haven't really experienced this issue since the first fix implementation in ACEX, so I am mostly putting it down to badly optimized missions/mods.

@Kexanone
Copy link
Member

ACEX Headless does transmit loadout to everyone as well, and that is for sure an improvement in network traffic. But I don't see how it can be enough of an improvement to "fix" the issue, not while there are a million way more intensive things being broadcasted.

I agree with that, but the more important improvement of the new implementation is that the loadouts are guaranteed to be defined when you restore them. Achilles did not check and wait for the loadouts broadcasted via public setVariable to be defined on the new owner.

@jonpas
Copy link
Contributor

jonpas commented Jan 11, 2021

Ah I see what you mean. That's actually a pretty good point.

@Kexanone
Copy link
Member

Actually, seeing now the ACEX implementation. The local EH combined with the appropriate checks via CBA_fnc_waitUntilAndExecute could be the best approach to tackle this problem in a general way. At this point, why not provide a common fix for all of us with CBA?

@jonpas
Copy link
Contributor

jonpas commented Jan 11, 2021

At this point, why not provide a common fix for all of us with CBA?

Sure, I think that would be in scope. Want to open a PR?

@Kexanone
Copy link
Member

Sure, I think that would be in scope. Want to open a PR?

Yeah, I'll work on one when I got time.
Would fit nicely into the new characters addon if you merge CBATeam/CBA_A3#1344 ;)

@jonpas
Copy link
Contributor

jonpas commented Jan 11, 2021

Awesome! 👍

Yeah, it's tagged for 3.16. But things are slow in this age of Arma 3.

This reverts commit bb41331.
@Kexanone
Copy link
Member

Reverted the fix in favor of CBATeam/CBA_A3#1406

@ampersand38
Copy link
Member Author

image

addons/editor/functions/fnc_toggleSelectionPreview.sqf Outdated Show resolved Hide resolved
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
addons/editor/functions/fnc_handleSelectionChanged.sqf Outdated Show resolved Hide resolved
* 0: Entity: group, marker, object, or waypoint <GROUP, STRING, OBJECT, ARRAY>
*
* Return Value:
* Handled <BOOL>
Copy link
Member

Choose a reason for hiding this comment

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

Can this event handler be overridden?

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't store the index of the event anywhere, but we did the same for all other curator event handlers. We could consider bridging them with CBA event handlers, but I would make a separate PR for such a change.

Kexanone and others added 2 commits January 24, 2021 00:56
Target:
if remote then curator
else
    if HCs then HC
    else server

HC scripts: if default going to HC then enabled, else disabled
@ampersand38
Copy link
Member Author

@mharis001 mharis001 modified the milestones: 1.10.0, 1.11.0 Feb 3, 2021
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
addons/modules/functions/fnc_moduleTransferOwnership.sqf Outdated Show resolved Hide resolved
],
[
"TOOLBOX",
LSTRING(ModuleTransferOwnership_HCScripts),
Copy link
Member

@CreepPork CreepPork Mar 4, 2021

Choose a reason for hiding this comment

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

Tooltip, at least, please? What is a HC script? To an end-user, no idea what this is and why I should enable it or disable it. And why is this a toggle, if it does something good why not auto it?

Copy link
Member Author

@ampersand38 ampersand38 Mar 4, 2021

Choose a reason for hiding this comment

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

It does auto-select based on the current locality:

  • If local, assume target will be HC and balancing will be enabled.
  • If remote, assume target will be Curator and balancing will be disabled.
    However I wanted to let user pick for edge cases.

What else can we do to make it easier to understand and use?
a1696ce

Copy link
Member

@mharis001 mharis001 left a comment

Choose a reason for hiding this comment

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

I would like to see this feature moved to its own component so it is easier to work on in the future. editor component already has a lot of responsibilities and it will be difficult to see everything involved in making this work.

Comment on lines +321 to +344
[QGVAR(transferOwnership), {
params ["_entities", "_target"];
if (!(_entities isEqualType [])) then {
_entities = [_entities];
};
private _clientID = 0;
if (_target isEqualType 0) then {
_clientID = _target;
};
if (_target isEqualType objNull) then {
_clientID = owner _target;
};
{
if (_x isEqualType grpNull) then {
_x setGroupOwner _clientID;
} else {
if (group _x == grpNull) then {
_x setOwner _clientID;
} else {
group _x setGroupOwner _clientID;
};
};
} forEach _entities;
}] call CBA_fnc_addEventHandler;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is big enough to warrant moving into a function/separate file.

Comment on lines +78 to +84
"TOOLBOX",
[LSTRING(ModuleTransferOwnership_HCScripts), LSTRING(ModuleTransferOwnership_HCScripts_Description)],
[parseNumber (_defaultTarget == 2), 1, 2, [
ELSTRING(common,Disabled),
ELSTRING(common,Enabled)
]],
true
Copy link
Member

Choose a reason for hiding this comment

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

Use TOOLBOX:ENABLED here.

{
drawIcon3D [
GVAR(lastModuleIcon),
GVAR(colour), getPosVisual _x, 1, 1, 0
Copy link
Member

Choose a reason for hiding this comment

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

getPosVisual returns AGLS, drawIcon3D expects AGL.

@mharis001 mharis001 modified the milestones: 1.11.0, 1.12.0 Jun 14, 2021
@mharis001 mharis001 modified the milestones: 1.12.0, 1.13.0 Sep 3, 2021
@mharis001 mharis001 modified the milestones: 1.13.0, Backlog Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants