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

Add configurable factory I/O + fix a few bugs #58

Merged
merged 21 commits into from
Aug 28, 2021

Conversation

caucow
Copy link

@caucow caucow commented Jun 6, 2021

Adds configurable factory IO as described in #54.

Also fixes diode power loop in #51.

Possible fix for #40? Maybe only partial fix? Redstone dust triggers might not be possible with Paper.

Fixes diode triggers activating factories they are not pointing into. Will need additional work if other directional blocks have the same problem.

Updated the main factoryinstance UI, fix for #9? How updated does it all need to be, recipes are in a scroller and other buttons line the bottom.

Depends on CMC PR 101

@Diet-Cola
Copy link

On test

@Diet-Cola
Copy link

Updated test

@Diet-Cola
Copy link

[04:19:23 ERROR]: Could not pass event PlayerInteractEvent to FactoryMod v2.5.2
java.lang.NullPointerException: Cannot invoke "com.github.igotyou.FactoryMod.factories.FurnCraftChestFactory$UiMenuMode.ordinal()" because "curMode" is null
        at com.github.igotyou.FactoryMod.interactionManager.FurnCraftChestInteractionManager.buildMenuModeCycleButton(FurnCraftChestInteractionManager.java:337) ~[?:?]
        at com.github.igotyou.FactoryMod.interactionManager.FurnCraftChestInteractionManager.buildRecipeInventory(FurnCraftChestInteractionManager.java:220) ~[?:?]
        at com.github.igotyou.FactoryMod.interactionManager.FurnCraftChestInteractionManager.leftClick(FurnCraftChestInteractionManager.java:200) ~[?:?]
        at com.github.igotyou.FactoryMod.listeners.FactoryModListener.playerInteract(FactoryModListener.java:163) ~[?:?]
        at com.destroystokyo.paper.event.executor.asm.generated.GeneratedEventExecutor318.execute(Unknown Source) ~[?:?]
        at org.bukkit.plugin.EventExecutor.lambda$create$1(EventExecutor.java:69) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:80) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:70) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:624) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at org.bukkit.craftbukkit.v1_16_R3.event.CraftEventFactory.callPlayerInteractEvent(CraftEventFactory.java:549) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at org.bukkit.craftbukkit.v1_16_R3.event.CraftEventFactory.callPlayerInteractEvent(CraftEventFactory.java:512) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at org.bukkit.craftbukkit.v1_16_R3.event.CraftEventFactory.callPlayerInteractEvent(CraftEventFactory.java:507) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.PlayerInteractManager.a(PlayerInteractManager.java:210) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.PlayerConnection.a(PlayerConnection.java:1732) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.PacketPlayInBlockDig.a(SourceFile:40) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.PacketPlayInBlockDig.a(SourceFile:10) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.PlayerConnectionUtils.lambda$ensureMainThread$1(PlayerConnectionUtils.java:55) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.TickTask.run(SourceFile:18) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.executeTask(IAsyncTaskHandler.java:136) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.IAsyncTaskHandlerReentrant.executeTask(SourceFile:23) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.executeNext(IAsyncTaskHandler.java:109) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.MinecraftServer.bb(MinecraftServer.java:1325) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.MinecraftServer.executeNext(MinecraftServer.java:1318) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.IAsyncTaskHandler.executeAll(IAsyncTaskHandler.java:95) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.MinecraftServer.a(MinecraftServer.java:1455) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.MinecraftServer.w(MinecraftServer.java:1134) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at net.minecraft.server.v1_16_R3.MinecraftServer.lambda$a$0(MinecraftServer.java:292) ~[patched_1.16.5.jar:git-Tuinity-"f7c4249"]
        at java.lang.Thread.run(Thread.java:831) [?:?]

caucow added 2 commits June 11, 2021 00:13
Default IO was only being initialized for old factories loaded from file, not new factories.
@Diet-Cola
Copy link

Updated test

@caucow
Copy link
Author

caucow commented Jun 13, 2021

Added configable fuelchests + limit options
defaults are

max_input_chests: 10
max_output_chests: 10
max_fuel_chests: 10
max_iof_chests: 15

@Diet-Cola
Copy link

Updated test

@Diet-Cola
Copy link

Minor note but if you dont mind updating your local paper Shadow and implementing the close() method in the MultiInventoryWrapper with this, same for your CMC PR with ClonedInventory

@caucow
Copy link
Author

caucow commented Jun 16, 2021

paper is better than this ffs
and why isn't close() just a default method... it can be a default method
updated (+cmc)

@Convoy20
Copy link

Convoy20 commented Aug 11, 2021

@caucow is the check below an issue?

@caucow
Copy link
Author

caucow commented Aug 11, 2021

@caucow is the check below an issue?

Build fails because Could not find artifact vg.civcraft.mc.civmodcore:CivModCore:jar:1.8.4
because

Depends on CMC PR 101

It's not a problem, CMC just needs its PRs merged before this can build properly.
At a bare minimum, CivClassic/CivModCore#101 and CivClassic/CivModCore#102 also needs to be pulled, and CMC's dependencies in jenkins/whatever CI (idk anything about CI rip) need updated so that PR will build.

Copy link
Member

@Maxopoly Maxopoly left a comment

Choose a reason for hiding this comment

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

Needs a lot of testing which I cant really comment on here, otherwise okay except for two minor things.

ArrayList<Inventory> invs = new ArrayList<>(12);
Block fblock = getFurnace();
BlockFace facing = getFacing();
for (BlockFace relativeFace : getFurnaceIOSelector().getInputs(facing)) {
Copy link
Member

Choose a reason for hiding this comment

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

This and all the copy pastes below should be extracted into a method

/**
* @author caucow
*/
public class DirectionMask {
Copy link
Member

Choose a reason for hiding this comment

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

Unless you're gonna store a few hundred million of these, please just forget about this C kind of optimization. Define an enum and use an EnumSet to generate both a human reaadable storage file and easier expandable code.

@caucow caucow requested a review from Maxopoly August 19, 2021 19:53
@caucow
Copy link
Author

caucow commented Aug 19, 2021

Needs a lot of testing which I cant really comment on here, otherwise okay except for two minor things.

I spent a lot of time in game testing the new features including chaining a number of the new factories together in different configurations (which lead to me fixing some of the other older bugs since they were interfering, then testing those fixes/discussing behavior in praxis), checking input/output/fuel being consumed (or not) when directions are toggled, making sure factories shut off when the MultiInv doesn't have required mats, some crude performance testing (perf hit scales linearly with number of items/chests in a MultiInv, hard config limits added to prevent compact factory lag machines, next to no extra overhead that wasn't already included), some stability testing (not much because the plugin is barely designed with fail cases and config/storage issues in mind), plus whatever time people on the CC test server spent with it. Is there anything specific you're concerned about?

Latest commit changes should probably get more testing (I'm out of town atm), most I did was make sure it compiled, run, let me make and configure factories, and properly saved/loaded/reloaded factory storage.

@Convoy20
Copy link

Newest build is on test

Copy link
Member

@Maxopoly Maxopoly left a comment

Choose a reason for hiding this comment

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

Resolves my complaints nicely

@Maxopoly
Copy link
Member

Maxopoly commented Aug 20, 2021

Might want to also include user friendly behavior for server which decide to not enable this feature. I know that you can just set it to 0 in the config, but mean rather making sure that the GUI and/or help messages aren't confusing and indicating towards something thats disabled.
Not terribly important though

- Make text in ioconfig screen more clear and noticeable
- Barriers for missing chests pop back after configuring
- Fix for factories not auto-selecting a new recipe if ingredients run out mid-run
- Fix for async-block-access on factory save
- General minor code cleanup and renaming
@wingzero54 wingzero54 merged commit a1fde29 into CivClassic:master Aug 28, 2021
wingzero54 added a commit to CivClassic/AnsibleSetup that referenced this pull request Aug 28, 2021
@caucow caucow deleted the configurable-io branch August 30, 2021 03:16
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.

5 participants