AttacqueSuperior / Engine

A customized fork for the modification Attacque Supérior of the open-source implementation of the Command & Conquer: Red Alert engine using .NET/Mono and OpenGL. Runs on Windows, Linux and Mac OS X.
http://attsup.swr-productions.com
GNU General Public License v3.0
16 stars 3 forks source link

Repair/Powerdown Mode: box-dragging to repair/powerdown units #95

Open dnqbob opened 4 years ago

dnqbob commented 4 years ago

Will it be welcomed here? I guess I should put the commit here as an alternative for global order buttons.

GraionDilach commented 4 years ago

I don't have the head value to review https://github.com/OpenRA/OpenRA/pull/17477 for now. However I need to think about how invasive the changes are, considering that the whole upstream suggestion to bring it here is to just avoid the maintenance costs themselves.

The criticism towards the C&C style controls in https://github.com/OpenRA/OpenRA/pull/17477#issuecomment-593054834 also opens warning bells in my head.

dnqbob commented 4 years ago

I guess maintenance costs here they don't want mainly because this function is worthless in default mods (whatever the reason) and maintaining such kind of code but never put into use is indeed somewhat ridiculous.

However, I think it is still upon you here to decide the code maintainance cost as well as its usage here, I will follow your idea on how to simplify if you want.

The criticism towards the C&C style controls in OpenRA#17477 (comment) also opens warning bells in my head

Just don't worry about this. Even if OpenRA take this direction it will be too late to change everything. If actor (includes unit and building) itself is simple enough, which have less than one action can be triggered by players (like power overload skill of reactor in General), C&C style controls can still take it but it is never convenient to achieve a Hero with more than two skills only under C&C style controls, so you can see C&C3 introduce a Unit UI to perform skill. If C&C3 take General's UI design(action slot UI), global order buttons can be intergrated, producing slot can be intergrated, command buttons (deployed, attack move, protect, stances) can be intergrated. A lot of UI space is saved, then some games like Warcraft3 can use addtional space show the actor status (like repairing, poisoned, healing).

GraionDilach commented 4 years ago

A conceptual issue I see here is that @dragunoff & @pchote rejects your feature on the feedback/experience https://github.com/OpenRA/OpenRA/pull/15839 generated, without acknowledging that many of the reasons they cite are (as well if not more) applicable to "flattening-selectable-priorities".

How I see is that https://github.com/OpenRA/OpenRA/pull/15839 is a dead end feature with a comparable level of UI confusion while also requiring modder assistance (the second commit) to supplement power users (and them alone) while introducing complication to newbies without no potential for the future besides making a precedent for inclusion of such modifiers for a single action and taking almost all the obvious button combinations for them. C&C3/RA3-styled UI action buttons are something which I've frequently seen modders mentioning as a daydream through the years and any steps towards such an UI should be preferred or at least considered as equal as such a pandering-to-powerusers dead end UI feature. I consider mass-selling as a usecase applicable for YR (Mastermind + Grinder combo, the slaves-to-Brutes GeneticMutator YR money exploit are two notable cases where it can come handy).

With this in mind, I consider both features equally worthwhile for upstream inclusion and I feel that if repair/select/powerdown box selection is rejected for conceptual reasons, these same reasons should also be applied retroactively to the selection flattening modifiers along to investigate their value.

I also don't find the idea to push this feature onto AS as an acceptable middle ground considering the above. Most of the AS features are planned with a mindset so that their potential isn't locked behind unplanned code paths but unplanned YAML interactions, it's not up to a followup code PR to unlock their true potential, and these features should encourage YAML modders to experiment with interactions instead. Hidden features with unhandled potentials sitting within the AS codebase is something I don't like the sound of, considering that these features are just wasted potential for the community in the longer term due to the general lack of interest/unwillingness of upstream even considering of transferring portions of the AS codebase (need to praise @Mailaender for his recent efforts for mediating though).

While I do believe this should belong to OpenRA and not to AS, I do see an option for downstream inclusion but such would need a different take altogether. One of the strategies I deploy to decrease maintenance burden is to minimalize the required changes within Mods.Common even at the cost of duplicating codes into Mods.AS, so the changes within GlobalButtonOrderGenerator would definitely require to live in a GlobalButtonOrderGeneratorAS duplicate along with the usecases and probably their widget consumers (this is not a real review, I just glossed the code over, I can be wrong that this would be enough) for any merge attempt for AS.

TL;DR: Usually OpenRA contains the features/codebase with potential waiting to be unlocked while AS is just a collection on top of this housing fancy "complete" dead end features. The conflict of interest here however is that OpenRA is willing to stick towards a dead end feature and openly suggest AS to house the progressive-with-unlockable-hidden-potential case instead.

GraionDilach commented 4 years ago

Adding the Needs Update label to mark that this depends upon the result of upstream discussions.

dnqbob commented 4 years ago

Thanks.

While I do believe this should belong to OpenRA and not to AS, I do see an option for downstream inclusion but such would need a different take altogether.

Except all above, the balance member think this will destroy APM and not agree using it on default mods, upstream, even if at last I drop most of the modifiers and the priorities.

so the changes within GlobalButtonOrderGenerator would definitely require to live in a GlobalButtonOrderGeneratorAS duplicate along with the usecases and probably their widget consumers

Orignal GlobalButtonOrderGenerator is very strict and only enough for classical C&C global order button and even only inherited by two classes (powedown and sell) at present, because repair function is more complicated after it can repair vehicle. I feel bad about it, so I rewrite the GlobalButtonOrderGenerator and it's derived classes to ensure any global order button that has similar design (let actors perform action while ignoring current selection collection set, one of C&C weirdest) can&should inherit this class.

GraionDilach commented 4 years ago

so the changes within GlobalButtonOrderGenerator would definitely require to live in a GlobalButtonOrderGeneratorAS duplicate along with the usecases and probably their widget consumers

Orignal GlobalButtonOrderGenerator is very strict and only enough for classical C&C global order button and even only inherited by two classes (powedown and sell) at present, because repair function is more complicated after it can repair vehicle. I feel bad about it, so I rewrite the GlobalButtonOrderGenerator and it's derived classes to ensure any global order button that has similar design (let actors perform action while ignoring current selection collection set, one of C&C weirdest) can&should inherit this class.

I'll have to ignore this detail for the sake of maintainance, because any future change in the logic might pose merge conflicts in the longer run due to the shared files. Having the code duplicated out ensures that the merges go down successfully and the errors crop up during compilation - where I can easily pinpoint the source of conflicts through the IDE instead of the difftool during merging.

dnqbob commented 4 years ago

Hi here. Thanks Nolt's advice I make a switcher class in my function, so YAML modders (as well as players) can turn on/off or set default value on switcher class in YAML /ingame. swicher

It can solve at least

Most of the AS features are planned with a mindset so that their potential isn't locked behind unplanned code paths but unplanned YAML interactions, it's not up to a followup code PR to unlock their true potential, and these features should encourage YAML modders to experiment with interactions instead.

Besides, I continue on expanding the function of powerdown. I guess OpenRA RA hardcore players will be more annoyed when they see this one:

power number

But I have to say it will be vital when a mod with variable power consuming buildings/units. For example,

poweroffunits

(I use a modified SP usecase to demostrate my idea, because OpenRA TS was too complicated to get a unit can be power-effected, as well as refactoring those YAML inheriting caused further bug, then I gave up. Hope Nolt won't mind this :( )

dnqbob commented 4 years ago

Found a bug in showing power-consuming of plug-in-building, fixed. But it requires modder to -- change some lines of "power condition" like GDI Component Tower of OpenRA TS.

Change from:

Power@turrets: RequiresCondition: !disabled&&(tower.vulcan || tower.rocket || tower.sam) Amount: -20

to

Power@turrets: RequiresCondition: tower.vulcan || tower.rocket || tower.sam Amount: -20 PauseOnCondition: disabled

Powerdown function can only find the amount of power-consuming in actived "power condition", not conditions that will be inactive when "disabled". However, actived "power condition" but paused when "disable" is OK and satisfies its orignal behaivior as well as the need of Powerdown function's numeral notification.

GraionDilach commented 3 years ago

@dnqbob Do I assume it right that https://github.com/OpenRA/OpenRA/pull/18769 was a step towards implementing this in downstream?

dnqbob commented 3 years ago

Nope, but it can make our job easier if we want a drag sell to send units to recycle building, and my functions here, otherwise we have to modify that class or make a new class.