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

Fake entities & minor changes #324

Closed
wants to merge 2 commits into from
Closed

Conversation

m9w
Copy link
Contributor

@m9w m9w commented Oct 22, 2023

It allow customize behavior of collector module & pathfinder without overriding methods these modules. Was added handling of loading raw bytecode. Also was suppressed showing instructions windows if bot running.

This changes required API changes darkbot-reloaded/DarkBotAPI#91

@m9w m9w changed the title Fake enitites + loader Fake entities + loader Oct 22, 2023
Copy link
Member

@Pablete1234 Pablete1234 left a comment

Choose a reason for hiding this comment

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

Generally looking good, just a few tweaks and i'd like to see the semantics of current FakeNpc be brought close together to the other fakes.

Ontop of that, while this is a great API to have, i think the bot itself should be keeping box/mine fakes without needing a behavior/module to do it.

Comment on lines 17 to 20
if (obj instanceof Module && !(obj instanceof TemporalModule)
&& !HeroManager.instance.main.isRunning()
) showInstructions(obj, fd.getName());
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 the pop-up should still appear if the bot is running, but what should change the behavior is as a result of what the change is. If the user changed module i think it's good that they get the pop-up, but i agree if it's an automatic change it shouldn't be shown.
I'm not exactly sure how to properly detect if it was automatic or not, since plugins can be the ones changing the config themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was added API method for changing config and this call suppressing showing instructions. Also was added default true setting property for suppress showing instructions in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implement via checking config value

src/main/java/eu/darkbot/api/game/entities/FakeEntity.java Outdated Show resolved Hide resolved
src/main/java/eu/darkbot/api/game/entities/FakeEntity.java Outdated Show resolved Hide resolved
src/main/java/eu/darkbot/api/game/entities/FakeEntity.java Outdated Show resolved Hide resolved
@m9w
Copy link
Contributor Author

m9w commented Dec 10, 2023

CAETest.java.txt
module for testing cross-applications events

@m9w m9w changed the title Fake entities + loader Fake entities & classloader & config API & cross-applications events Dec 10, 2023
@Pablete1234
Copy link
Member

Please rebase the changes instead of merging, it makes reviewing much harder as all the changes already in the bot since the start of the PR now appear as changes, so its hard to tell what's new and what's not

@m9w
Copy link
Contributor Author

m9w commented Dec 11, 2023

@Pablete1234 changes was rebased, chery-picked, squashed and force pushed.

private boolean isRemoveWhenAttemptSelect;

public FakeBox(String boxName, Location loc, long removeDistance, long keepAlive, boolean isRemoveWhenAttemptSelect) {
super(Integer.MIN_VALUE);
Copy link
Member

Choose a reason for hiding this comment

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

should probably use different IDs for these, could probably just hold a global fake entity id counter and keep on using extremely low numbers (counting up from min value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about random id? super(ThreadLocalRandom.current().nextInt(Integer.MIN_VALUE, -1000));
but I found where it can be used and not sure that it's matter

Copy link
Contributor

Choose a reason for hiding this comment

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

lets not use randomly generated value here, a defined value or add boolean check to tag them as a "fake" to show the entity is as fake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets not use randomly generated value here, a defined value or add boolean check to tag them as a "fake" to show the entity is as fake

it's random negative values. For detecting fake can use https://github.com/darkbot-reloaded/DarkBotAPI/pull/91/files#diff-21bb577e67916994b49cca980705fdb72d7f47329f09bac1596e907da599c906R12

Copy link
Member

Choose a reason for hiding this comment

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

So there's two issues here: the game itself already uses negative ids for some stuff, starts with -1, then -2, etc, and it likely will reach -1000 if you run the bot for long enough.

Secondly, with random values it's possible to end with a collision, and i'd want to avoid that.

I don't see the issue in simply having private static int CURR_ID = Integer.MIN_VALUE; and assigning id as super(CURR_ID++), which should generate unique, sequantial, negative ids for everything.

And yeah, as to what anjesh mentioned, the ID is not a way to determine if they're fake entities, check via instanceOf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if use private static int CURR_ID = Integer.MIN_VALUE; and super(CURR_ID++) than we have 2 147 483 648 negative values so when we create more entities then IDs will be positive. I think it not good thing.

P.S. I made it like it wrote in FakeNPC class
image

Copy link
Member

Choose a reason for hiding this comment

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

creating over 2 bilions of entities seems unreal for me, even with runtime over a year
year have over 30 milions of seconds, so you have to create ~70 entities every second to fill it up in a year

Copy link
Member

@Pablete1234 Pablete1234 Dec 12, 2023

Choose a reason for hiding this comment

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

Sorry i actually meant CURR_ID-- not ++.

The thing about FakeNPC is it's guaranteed to only exist once, its used as a singleton

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented

@m9w m9w changed the title Fake entities & classloader & config API & cross-applications events Fake entities & classloader & config API Jan 18, 2024
@m9w m9w changed the title Fake entities & classloader & config API Fake entities & minor changes Jan 18, 2024
Comment on lines +19 to +20
&& ConfigEntity.INSTANCE.getConfig().BOT_SETTINGS.OTHER.SHOW_INSTRUCTIONS
) showInstructions(obj, fd.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&& ConfigEntity.INSTANCE.getConfig().BOT_SETTINGS.OTHER.SHOW_INSTRUCTIONS
) showInstructions(obj, fd.getName());
&& ConfigEntity.INSTANCE.getConfig().BOT_SETTINGS.OTHER.SHOW_INSTRUCTIONS)
showInstructions(obj, fd.getName());

minor formatting change

@@ -128,6 +129,24 @@ private void onEntityCreate(Entity entity) {
this.eventBroker.sendEvent(new EntityCreateEvent(entity));
}

public FakeEntity.FakeMine createFakeMine(int typeId, eu.darkbot.api.game.other.Location loc, long removeDistance, long keepAlive) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public FakeEntity.FakeMine createFakeMine(int typeId, eu.darkbot.api.game.other.Location loc, long removeDistance, long keepAlive) {
public FakeEntity.FakeMine createFakeMine(int typeId, eu.darkbot.api.game.other.Location loc, long removeDistance, long keepAlive) {

minor formatting change

@@ -141,6 +141,7 @@ public GuiManager(Main main, PluginAPI pluginAPI, RepairManager repairManager) {
this.assembly = register("assembly");

register("ggBuilder", GateSpinnerGui.class);
register("refinement_count");
Copy link
Member

Choose a reason for hiding this comment

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

does this not belong to a diff PR?

@@ -135,4 +141,61 @@ public Beacon(int id, long address) {
super(id, address);
}
}

public static class FakeBox extends Box implements eu.darkbot.api.game.entities.FakeEntity.FakeBox {
private static int CURR_ID = Integer.MIN_VALUE;
Copy link
Member

Choose a reason for hiding this comment

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

needs to be shared by all fake entities, to avoid duplicate ids. Just have this sit in some util where you can EntityUtil.allocateFakeId() and it returns CURR_ID++ or similar

Comment on lines +58 to +62
try {
Popups.setDefaultIcon(MainGui.ICON);
} catch (Exception e) {
e.printStackTrace();
}
Copy link
Member

Choose a reason for hiding this comment

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

i wonder, what's the motive behind this change? is it throwing an exception under some systems? if so, which exception?

@Pablete1234
Copy link
Member

Given the commit was abandoned i've re-coded it, implemented the updated fake entities api, and also introduced the option for instructions.
If you feel other important changes were left out feel free to open separate PRs for each of the feature changes you want us to consider, avoid making mega-prs which merge different features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants