-
Notifications
You must be signed in to change notification settings - Fork 8
Add folia support #77
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
base: master
Are you sure you want to change the base?
Conversation
Jsinco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks fine at a glance, just proposing changes to ticking those cauldrons
| .orElse(Color.GRAY); | ||
|
|
||
| this.playBrewingEffects(); | ||
| Optional<World> optionalWorld = BukkitAdapter.toWorld(location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like Nadwey said, this shouldn't be creating a fixed-rate scheduler. I also don't think the folia scheduler object is needed here either
| Optional<World> optionalWorld = BukkitAdapter.toWorld(location); | |
| Location bukkitLocation = BukkitAdapter.toLocation(location); | |
| SinglePositionStructure structure = this; | |
| Bukkit.getRegionScheduler().run(TheBrewingProject.getInstance(), bukkitLocation, (task) -> { | |
| if (!BlockUtil.isChunkLoaded(location)) { | |
| return; | |
| } | |
| if (!Tag.CAULDRONS.isTagged(getBlock().getType()) || getBlock().getType() == Material.CAULDRON) { | |
| ListenerUtil.removeActiveSinglePositionStructure(structure, TheBrewingProject.getInstance().getBreweryRegistry(), TheBrewingProject.getInstance().getDatabase()); | |
| return; | |
| } | |
| hot = isHeatSource(getBlock().getRelative(BlockFace.DOWN)); | |
| recalculateBrewTime(); | |
| Color baseParticleColor = computeBaseParticleColor(getBlock()); | |
| Optional<Recipe<ItemStack>> recipeOptional = brew.closestRecipe(TheBrewingProject.getInstance().getRecipeRegistry()); | |
| Color resultColor = computeResultColor(recipeOptional); | |
| particleColor = recipeOptional.map(recipe -> computeParticleColor(baseParticleColor, resultColor, recipe)) | |
| .orElse(Color.GRAY); | |
| playBrewingEffects(); | |
| }); |
| @@ -0,0 +1,156 @@ | |||
| package dev.jsinco.brewery.bukkit.util; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidentally left this comment out of my review:
I don't think this object needs to be here
Thorinwasher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to have everything that is done on inventories in the proper thread, this is important because otherwise there's nothing saying what or how something could get duped. See
TheBrewingProject/bukkit/src/main/java/dev/jsinco/brewery/bukkit/TheBrewingProject.java
Lines 258 to 259 in e9dcc13
| List.copyOf(breweryRegistry.<BukkitBarrel>getOpened(StructureType.BARREL)).forEach(Barrel::tickInventory); | |
| List.copyOf(breweryRegistry.<BukkitDistillery>getOpened(StructureType.DISTILLERY)).forEach(Distillery::tickInventory); |
| Bukkit.getGlobalRegionScheduler().runAtFixedRate(this, task -> this.updateStructures(), 1, 1); | ||
| Bukkit.getGlobalRegionScheduler().runAtFixedRate(this, task -> this.otherTicking(), 1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods would also by themself need to delegate to probably something like the local region scheduler for each structure.
| .orElse(Color.GRAY); | ||
| playBrewingEffects(); | ||
| } | ||
| }.runAtFixedRate(TheBrewingProject.getInstance(), 20, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to cause a memory leak. You could at least simplify it to runNow, no need for it to be more complicated than that
| Bukkit.getGlobalRegionScheduler().runAtFixedRate(this, task -> this.updateStructures(), 20, 1); | ||
| Bukkit.getGlobalRegionScheduler().runAtFixedRate(this, task -> this.otherTicking(), 20, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you delaying everything by 1 second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is everything is 1 tick, idk how u do your system tick and how u are watch then processing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.otherTicking(), 20, 1);
^ This is one second delay before everything starts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.otherTicking(), 20, 1); ^ This is one second delay before everything starts
oh, if you mean that - I'm sorry
| TheBrewingProject.getInstance().getActiveEventsRegistry().registerActiveEvent(playerUuid, event, Config.PUKE_TIME); | ||
| Bukkit.getScheduler().runTaskTimer(TheBrewingProject.getInstance(), pukeHandler::tick, 0, 1); | ||
|
|
||
| player.getScheduler().runAtFixedRate(TheBrewingProject.getInstance(), pukeHandler::tick,null ,1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the delay from 0 ticks to 1 ticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the delay from 0 ticks to 1 ticks?
when folia scheduler don't allow set 0 on delay ticks. this is must be > 0 like 1 or more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I did not know that. Good to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like something:
[11:57:23] [Server thread/ERROR]: Error occurred while enabling TheBrewingProject v1.7.0 (Is it up to date?) -- | java.lang.IllegalArgumentException: Initial delay ticks may not be <= 0
I will look later that |
…o feat/folia-support # Conflicts: # bukkit/src/main/java/dev/jsinco/brewery/bukkit/breweries/BukkitCauldron.java # bukkit/src/main/java/dev/jsinco/brewery/bukkit/effect/event/NamedDrunkEventExecutor.java
Add runFolia task to BreweryTeam#77
First support for folia in this plugin. There may still be areas that require revision.