Added theoretical reinforcements

[xFier]

Required to fix some issues with the event priority system in Bastions

[ProgrammerDan]

Can you describe a bit more the source of this problem and what this fix addresses in the Bastion system? I've lost the thread on this, sorry.

[DevotedServant]

Can one of the admins verify this patch? @ProgrammerDan @Maxopoly

"ok to test" will build any commits made to this PR
"test this please" will build this PR once
"retest this please" will make another single build for this PR
"add to whitelist" will whitelist the creator of this PR and make jenkins automatically build any PRs made by him

[ProgrammerDan]

From interview with xFier:

"That theoretical reinforcement stuff is there to solve the two different types of bastions not being damaged at the same time (Ones that stop reinforcements and ones that stop block placements)

Basically the issue is tracked via:
A block placement is required for a reinforcement to be made
If the block placement is cancelled the reinforcement code never runs.
Because the reinforcement code never runs, it can never be cancelled thus you cannot damage both at the same time.

My idea behind the theoretical reinforcement was that we could check if a reinforcement could have been made (thus allowing us to say it shouldn't later on because the bastion blocks it) without actually making it & risking reinforcing air or other stuff like that"

relates to PR CivClassic/Bastion#5 over in Bastion.

To reproduce conditions:

"...if you have a Vault Bastion & A City Bastion close enough together such that they intersect. Using /ctf will obviously allow you to place a block & reinforce it at the same time.

When using this where the two bastions intersect you will not be able to damage both of them at the same time. I am pretty sure there was a debug tool or something that shows bastion health"

Thanks xFier!

[Maxopoly]

This is bad, because:

• It duplicates the logic instead of using the same logic both for actual reinforcements and these "theoretical ones", thus setting up for future bugs due to only changing one

• It calls a ReinforcementCreationEvent when no reinforcement is actually created

Generally I dont think it's a good idea for Bastion to be checking "what would Citadel do". Instead I would propose one of the following:

Set ignoreCancelled=false on the reinforcement creation listener, but in it's last step check whether the event has already been cancelled. If it was, call a AttemptedReinforcementCreationEvent, otherwise proceed as usual

---

Modify the listeners in Bastion, so damaging a Bastion by placement will always damage all Bastions within reach of the placed block, which the player does not have perms on.

---

I would heavily favor the second option, as it's a self contained change within Bastion

[ProgrammerDan]

Some great options here. I agree on the 2nd as well, that's where I was leading in my head, but I need to re-review the code a bit more before I could reply with confidence. Thanks!

…

On Wed, Sep 25, 2019 at 7:53 PM Maxopoly ***@***.***> wrote: This is bad, because: - It duplicates the logic instead of using the same logic both for actual reinforcements and these "theoretical ones", thus setting up for future bugs due to only changing one - It calls a ReinforcementCreationEvent when no reinforcement is actually created Generally I dont think it's a good idea for Bastion to be checking "what would Citadel do". Instead I would propose one of the following: Set ignoreCancelled=false on the reinforcement creation listener, but in it's last step check whether the event has already been cancelled. If it was, call a AttemptedReinforcementCreationEvent, otherwise proceed as usual ------------------------------ Modify the listeners in Bastion, so damaging a Bastion by placement will always damage all Bastions within reach of the placed block, which the player does not have perms on. ------------------------------ I would heavily favor the second option, as it's a self contained change within Bastion — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60?email_source=notifications&email_token=AADD5DRG4S3CUKX7EHNLZJDQLP2YLA5CNFSM4IMN67DKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7TZA7Y#issuecomment-535269503>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADD5DTPRCPGB7MBJARY7T3QLP2YLANCNFSM4IMN67DA> .