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

Adjust extractor capping #3484

Merged
merged 18 commits into from
Oct 24, 2021

Conversation

Garanas
Copy link
Member

@Garanas Garanas commented Oct 15, 2021

This PR is based on #3483 which is based on this forum topic.

image

The current implementation is best described by looking at the changes of this PR, but for clarities sake:

  • Assuming all engineers can build the first viable storage will queue that storage to build.
  • Anything that can build but not that storage will assist one of the engineers.

Sadly, the function IssueBuildMobile has some unexpected behavior: instead of giving all the units the same order (as you'd expect when doing a similar action as a player) it gives the orders to the nearest engineer to each structure. E.g., if you have all your engineers to the north of an extractor then only one gets to build all the storages. If you have them surrounding the extractor then four engineers start building each of the storages individually. Both are not acceptable.

Instead - we now queue the same order for all valid engineers. In turn, they all build the same storage. But they do not share the same build queue, e.g., manipulating the build queue (by removing a storage from the queue) is not possible when having multiple engineers selected, and it doesn't do the job when you only remove it from the queue of one engineer. Long story short: you'd need to give all the engineers a new order to cancel the storages.

@KionX Can you find out what the table (last argument) of IssueBuildMobile is supposed to do? It needs to be either { } or { {x, y} } where x and y are some number - but it doesn't appear to be doing anything.

@Garanas Garanas linked an issue Oct 15, 2021 that may be closed by this pull request
@Garanas Garanas added this to the 3726 milestone Oct 15, 2021
@Garanas Garanas changed the title Adjusts how storages are build Adjust extractor capping Oct 15, 2021
for key, location in locations do
if CanBuildInSpot(mex, msid, location) then
IssueBuildMobile({builder}, location, msid, {})
for _, builder in builders do
IssueBuildMobile({builder}, location, msid, {})
Copy link
Member

Choose a reason for hiding this comment

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

why not IssueBuildMobile(builders, location, msid, {}) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of:

Sadly, the function IssueBuildMobile has some unexpected behavior: instead of giving all the units the same order (as you'd expect when doing a similar action as a player) it gives the orders to the nearest engineer to each structure. E.g., if you have all your engineers to the north of an extractor then only one gets to build all the storages. If you have them surrounding the extractor then four engineers start building each of the storages individually. Both are not acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, thanks

@KionX
Copy link
Collaborator

KionX commented Oct 15, 2021

Last argument is named ListOfCells, format is { {x, z}, ... }, used by game, set also inside brain:BuildStructure.
The purpose is unknown. Not sure, but think this parameter is broken in IssueBuildMobile.

@Garanas
Copy link
Member Author

Garanas commented Oct 17, 2021

Thank you for looking into it - that means that it wouldn't fix the issue described in my post (instead of all engineers doing the same order, the nearest are chosen and the rest is ignored).

A few more ideas after WhenDayBreaks approached me:

  • 4 clicks to mass fab cap a T3 extractor
  • 2 clicks to mass storage cap a t3 fabricator
  • 2 clicks to t1 power gen cap a radar
  • 2 clicks to t1 power gen cap a t2 artillery

(see down below)

These are not as volatile as capping a pgen with energy storages and they're regularly used, but then through templates.

@Garanas Garanas marked this pull request as draft October 17, 2021 11:36
Instead of pre-defining all layers, you now make a callback per layer.
@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

image

It is working - see also the list of things you can now cap down below.

Behavior

The things we can cap and the number of clicks:

  • 2 clicks to mass fab cap a T3 extractor
  • 1 click to mass storage cap a t3 fabricator
  • 1 click to t1 power gen cap a radar
  • 1 click to t1 power gen cap a t2 artillery

When there are engineers of multiple factions in a selection the faction with the most engineers is chosen as the 'main builders', engineers of other factions (or units that can't build storages, poor ACU) assist one of the chosen engineers.

Note that a choice here was made: instead of looking at the tech level of the engineers or the build power, the one with the most engineers is chosen. In turn the code significantly easier / robust - if people want the highest tech engineer to take the lead then please make a post and provide rationale as to why it is better.

Performance

And of course: the approach is significantly more optimized in terms of table allocations. I don't think it will have a noticeable impact on the game performance, but everything helps.

Question

And last but certainly not least: do we want the ability to queue the capping of a structure while it is building? E.g., that you can click a t2 mass extractor while it is building and queue up its storages?

Answer: turns out it is not possible. The blueprint field is not set when repairing something - we need that in order to determine the viability of capping.

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

Will it be possible to disable the assist order somehow?
This was the original disadvantage.

@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

The assist order with the extractor? For what reason?

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

Of course not. assistance to engineers .

You wrote
When there are engineers of multiple factions in a selection the faction with the most engineers is chosen as the 'main builders', engineers of other factions (or units that can't build storages, poor ACU) assist one of the chosen engineers.

Maybe you can do it as an add-on. Everyone will be able to edit the code as they like.

@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

We have three situations:

  • When all engineers are of the same faction, they can all build the same storage. No assisting happening.
  • When you have engineers of two factions, one must assist the other as they can't build the same storages.
  • When you have engineers of one faction and units that can't build the storage (kennel drones, ACU) then they must assist an engineer as they can't build the storages themselves.

(See below)

What are you proposing we do different?

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

in all three situations, there is no order to assist.

Builders who cannot build buildings do not receive orders at all.
we select a group with a large number of types of builders, and build buildings.

@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

I'm not sure if there is a general consensus for that - what are the opinions of other people? I could also make this a game option (as part of the options menu) but it feels strange to add this (to assist or not to assist) as a separate option.

@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

And how about capping T1 Point Defense?

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

I think no consensus is possible. There will be endless arguments.
If you can do that separete option, it'll be fine for everyone.
It will be great. awsome

I would immediately build a cannon in the construction menu with capping. for default.
And did not make additional options. and radar or t2 artillery with pgen.
because it is cheap.

@Garanas
Copy link
Member Author

Garanas commented Oct 20, 2021

Adding options for the sake of options isn't favorable either. There is no rush for this PR as I am trying to finish it before Monday, lets see what other people think on the matter.

I would immediately build a cannon in the construction menu with capping. for default.
This is quite a different feature and will not be part of this PR.

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

Good. but I would make an option now. and did not return to this anymore.
Unless, of course, it's not entirely difficult.

@Sutrinc
Copy link

Sutrinc commented Oct 20, 2021

By the way.
an assisting order is a bad idea.
if you link the commander to the engineer. then send the engineer to the other side of the map, then the commander will follow him. it will be bad.

and other engineers will go too. that's exactly what they wanted to get away from.

@Tagada14
Copy link
Collaborator

I see no reason for adding such an option, if you select your ACU with engineers and double click the mex to make storages then it means you want the ACU to make the storages so it should be assisting.

@Tagada14
Copy link
Collaborator

Tagada14 commented Oct 21, 2021

Another issue with the assistance of engineers is the problem when you have a couple of engies assisting a mex already and when you issue a new assist command on the mex with a new engineer (that was not assisting the mex) sometimes it queues the storages even though it shouldn't.
Edit: Can't reproduce this, I guess it's just the fact that when the game lags you sometimes click twice.

@Sutrinc
Copy link

Sutrinc commented Oct 21, 2021

The task was to remove the order for assisting.
It turns out that you could do nothing at all. )))))

@Garanas
Copy link
Member Author

Garanas commented Oct 21, 2021

There is no task - and if any - it is to improve the current situation. And that is what we're doing here.

@Sutrinc
Copy link

Sutrinc commented Oct 21, 2021

I don't know the language well. the translator suggested so.

@Sutrinc
Copy link

Sutrinc commented Oct 21, 2021

and then. everything remained as it was.
it's a pity

@Garanas
Copy link
Member Author

Garanas commented Oct 21, 2021

That is fundamentally not true - the assist order is gone when you have a bunch of builders that can all the build capping structure. As stated in a previous post

@Garanas Garanas marked this pull request as ready for review October 23, 2021 15:00
@Sutrinc
Copy link

Sutrinc commented Oct 23, 2021

))))))
asked to remove the order for assistance.)) you worked for 2 weeks. we discussed 2 weeks. As a result, the order for assistance remained. and there is no way to remove it again. )))))
That's just funny. ))))))
you're strange.

@Garanas
Copy link
Member Author

Garanas commented Oct 23, 2021

And the order for assistance is removed for the typical case - the one that is used 99% of the time when you have a group of engineers that can build the same structure that is used for capping. I'd say you got exactly what you wanted.

@MrRowey
Copy link
Member

MrRowey commented Oct 23, 2021

this covers these 2 issues #1869 #1871

@Garanas Garanas linked an issue Oct 23, 2021 that may be closed by this pull request
@Garanas
Copy link
Member Author

Garanas commented Oct 23, 2021

Good find - it mentions the following list:

  • (ok) build 4 T1 pgens around T2 Radar structure (when not upgrading)
  • (ok) build 4 T1 pgens around T3 Omin structure
  • (ok) build 4 T1 pgens around T2 Artillery structure
  • (nok) build 4 T3 pgens around T3 mass fabs
  • (nok) build 4 T3 or 16 T1 pgens around T3 Artillery structure
  • (ok) build 8 wall sections around T1 Point Defense structure
  • (nok) build 4 or 8 T2 mass fabs around capped T3 mex

When discussing this feature with WhenDayBreaks we decided against using larger structures (anything with a larger footprint than a storage) because the amount of APM you save with that is minimal and the implementation requirements become significant.

Do you want me to look into capping a t1 point defense?

@Garanas Garanas linked an issue Oct 23, 2021 that may be closed by this pull request
@Garanas
Copy link
Member Author

Garanas commented Oct 24, 2021

I've added support for a T1 point defense and I've made sure that it is harder to accidentally cap structures multiple times due to lag.

@Garanas
Copy link
Member Author

Garanas commented Oct 24, 2021

Now we'd need to change the option description in the game options 😄 .

@Garanas Garanas merged commit 8761bdd into FAForever:deploy/fafdevelop Oct 24, 2021
@Garanas Garanas deleted the adjust-extractor-logic branch October 24, 2021 19:02
@ChessBerry
Copy link
Contributor

Great stuff being done here as always and after playing around with it a bit on faf develop, I got some suggestions as well.
I'll just link my forum post to avoid having the same discussion in two different places:
https://forum.faforever.com/topic/2714/the-function-of-automatically-building-mass-storage-around-the-miners/34

Garanas added a commit that referenced this pull request Nov 21, 2021
Garanas added a commit that referenced this pull request Nov 26, 2021
* Adjusts how storages are build

* Adding support for arbitrary capping

* Refactoring mass extractor capping code

* Refactor callback logic

Instead of pre-defining all layers, you now make a callback per layer.

* Refactor for documentation and performance

* Add documentation, remove logging

* Tweak behavior of capping

* Update documentation (with thanks to KionX)

* Add support for T1 point defense

* Small performance tweak

* Use skirt size instead of footprint size

* Improve performance, add basic exploit prevention

* Adjust double clicking logic slightly

* Adjust logic slightly

* Remove log statements

* Add proper check in case we have an invalid layer due to mods

* Bits and pieces of feedback from Balthazar

* Fix typo
Garanas added a commit that referenced this pull request Nov 28, 2021
* Adjusts how storages are build

* Adding support for arbitrary capping

* Refactoring mass extractor capping code

* Refactor callback logic

Instead of pre-defining all layers, you now make a callback per layer.

* Refactor for documentation and performance

* Add documentation, remove logging

* Tweak behavior of capping

* Update documentation (with thanks to KionX)

* Add support for T1 point defense

* Small performance tweak

* Use skirt size instead of footprint size

* Improve performance, add basic exploit prevention

* Adjust double clicking logic slightly

* Adjust logic slightly

* Remove log statements

* Add proper check in case we have an invalid layer due to mods

* Bits and pieces of feedback from Balthazar

* Fix typo
Garanas added a commit that referenced this pull request Dec 3, 2021
* Adjusts how storages are build

* Adding support for arbitrary capping

* Refactoring mass extractor capping code

* Refactor callback logic

Instead of pre-defining all layers, you now make a callback per layer.

* Refactor for documentation and performance

* Add documentation, remove logging

* Tweak behavior of capping

* Update documentation (with thanks to KionX)

* Add support for T1 point defense

* Small performance tweak

* Use skirt size instead of footprint size

* Improve performance, add basic exploit prevention

* Adjust double clicking logic slightly

* Adjust logic slightly

* Remove log statements

* Add proper check in case we have an invalid layer due to mods

* Bits and pieces of feedback from Balthazar

* Fix typo
Garanas added a commit that referenced this pull request Dec 11, 2021
Garanas added a commit that referenced this pull request Dec 20, 2021
* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Update changelog

* Add more info for 3316

* Update changelog for #3417

* Added #3522 to changelog

* Add #3523 to changelog

* Add #3349 to the changelog

* Add #3440 and #3512

* Add #3461 to the changelog

* Update #3461 in changelog

* Add #3419 to the changelog

* Add #3525 to the changelog

* Add #3526 to the changelog

* Add #3490 to the changelog

* Add #3527 to the changelog

* Add description for #3527 to changelog

* Improve description of #3490 to changelog

* Add #3528 to the changelog

* Add #3531 to the changelog

* Add #3535 to the changelog

* Add #3543 to the changelog

* Add #3533 to the changelog

* Add #3411 to the changelog

* Add #3552 to the changelog

* Add #3550 to the changelog

* Update wording of #3447 in the changelog

* Update wording of #3484 in the changelog

* Add #3554 to the changelog

* Add #3557 and #3558 to the changelog

* Add #3582 to the changelog

* Add #3581 to the changelog

* Add #3587 and #3589 to the changelog

* Add #3600 and #3601 to the changelog

* Add #3599 and #3598 to the changelog

* Add #3596 to the changelog

* Add #3590, #3588 and #3586 to the changelog

* Add #3567 to the changelog

* Add #3597 to the changelog

* Add suggestions of Rowey

* Add #3606 to the changelog

* Add #3605 to the changelog

* Add #3604 to the changelog

* Refactor changelog

* Refactor changelog

* Fix version of changelog

* Refactor description: only applies to t2 artillery

* Add #3607 to the changelog

* Add translator

* Add #3480 to the changelog

* Add #3610 to the changelog

* Add #3609 to the changelog

* Add #3612, #3616, #3618, #3614 and #3613 to the changelog

* Add #3602 to the changelog

* Update changelog, add it to the game

* Remove temp changelog

* Add message from game team
@Garanas Garanas mentioned this pull request Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants