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

Acidblock improvements #606

Closed
wants to merge 6 commits into from
Closed

Conversation

kickylol
Copy link
Contributor

@kickylol kickylol commented Aug 23, 2024

Addresses #492

  • there is now a timer for each block being acided which is now shown in /cti
  • acid timers now use the lower reinforcement if possible
  • multifaced acidblocks now only acid what is ready
  • /cti also now warns you if an acid is going to fail
  • you also get a warning for placing an acidblock that would fail
  • added an acidblock section to /ctdl
  • acidblocks can now acid other acidblocks, but will skip this if they are on the same group

New messages in chat and cti for when acids are going to fail with new checks for all faces of acid block. Multifaced acidblocks now choose the weakest reinforcement being acided to base its timer on.
@@ -92,4 +108,21 @@ private List<IClickable> getClicks(List<ReinforcementType> types, boolean allowe

return clickables;
}

private List<IClickable> getAcidTypeClicks(List<AcidType> acidTypes) {
List<IClickable> clickables = new LinkedList<>();
Copy link
Contributor

@awoo-civ awoo-civ Aug 23, 2024

Choose a reason for hiding this comment

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

use an ArrayList by default, unless you know you benefit from linked lists specifically (unlikely)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh I just tried to make it as similar to the already existing getClicks code as possible, the previous one used linked lists so ¯_(ツ)_/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

awoos passionate hatred for linkedlists can be seen in praxis civ-dev

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with awoo, you can even pre-size it with new ArrayList<>(acidTypes.length())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have killed the linkedlist

Copy link
Contributor

@awoo-civ awoo-civ left a comment

Choose a reason for hiding this comment

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

(dumb github ui)

@kickylol
Copy link
Contributor Author

all the changes listed above seem to be working fine now, but somebody testing it themselves would be appreciated

}
if (
targetBlockRein != null
&& !MaterialUtils.isAir(relativeBlock.getType())
Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh

Copy link
Contributor

@okx-code okx-code left a comment

Choose a reason for hiding this comment

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

Looks good overall, but a few small changes would be nice

@@ -92,4 +108,21 @@ private List<IClickable> getClicks(List<ReinforcementType> types, boolean allowe

return clickables;
}

private List<IClickable> getAcidTypeClicks(List<AcidType> acidTypes) {
List<IClickable> clickables = new LinkedList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with awoo, you can even pre-size it with new ArrayList<>(acidTypes.length())

@RedDevel2
Copy link
Contributor

Was this ready?

if (
relativeReinforcement == null
|| !relativeReinforcement.getType().canBeReinforced(relativeBlock.getType())
|| acidMan.isPossibleAcidBlock(relativeBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't acid blocks be able to affect other acid blocks now? This condition should be consistent with /cti

@okx-code
Copy link
Contributor

okx-code commented Nov 1, 2024

closing as abandoned

@okx-code okx-code closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Release
Development

Successfully merging this pull request may close these issues.

5 participants