-
Notifications
You must be signed in to change notification settings - Fork 9
[ref:vaan/feature/crucible] Crucible #449
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
|
The linked issue should be accomplished by allowing melting rocks into a smeltery imo |
|
Instead of adding a whole new machine |
That feels like shooting ducks with a cannon tho |
|
LordIdra
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.
This is almost identical to the mixing pot as it stands.
I would suggest instead to do what 'modded' crucibles do, where items that get thrown in are consumed (one by one) and then slowly melted. So you throw cobblestone in, and over 25 seconds, 4 mb of lava are added per second. Then, it's ready to consume the next cobblestone you throw in. (And also produces smoke gradually as it goes).
This makes balancing much easier and differentiates this enough to make it worth having alongside the mixing pot
Ok did something very similar to modded, instead of having it tick every second and add it gradually (which with many crucibles it might be unnecessarily performance intensive), I made it melt a block every X ticks, just like modded crucibles depending on the heat source below, it will change the melting speed |
|
Requires pylonmc/pylon-core#512 |
|
(changed name so itll compile) |
|
I think it is done? |
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.
Sorry, have realised I forgot to review this in my last PR crusade
Nice work on this, it looks better now. A couple of general things
- The fluid being displayed as water when you're melting lava is really awkward. I don't know if this PR is the place to address it given it is also a mixing pot issue as well but thought I'd mention it in case. (Open an issue if you don't want to address it here)
- Fire / soul fire does not display correctly in the guide (again not sure if this is an issue for this PR, open an issue if not)
- The particles are afaict the exact same as the mixing pot. I don't think they really suit the crucible and it feels a bit unpolished.
- I would probably avoid showing the items that can be used to heat the crucible. We can add a separate page for this when the disambiguation system is in.
- It would be great to have some way to see what is inside the crucible waiting to be melted. There's no way to know ATM. Maybe WAILA?
- Fluid output does not appear to work. Not sure why:
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/Crucible.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/Crucible.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/recipes/CrucibleRecipe.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/Crucible.java
Outdated
Show resolved
Hide resolved
|
Another thing rq - I think it makes more sense for the smelt time to be an attribute of the recipe than the crucible |
This is also for the mixing pot, maybe we can make it similar to how tank behave for both of them, but maybe in another issue
It is intended, we use barrier as fallback, there is no item to display fire anymore
Yeah, what should i use? maybe lava particles? will experiment this once the PR is done and working
So in the future?
I can display {ITEMNAME} x{AMOUNT}, I hope the WAILA won't get too messy
Not sure what you are doing with that, but I am using fluid tanks and it works Also noticed that the getTickInterval doesn't work anymore as expected, maybe there was a change idk, how it is saved as a constant instead in the code that handles tickers, might want to undo it back to allow more flexible behaviours such as this, otherwise i will need to have it unnecessarly tick a lot of often just to run sleeps inside |
I use flint and steel here for the mixing pot iirc. Might be good to fall back to that instead for [soul] fire? This looks like an error from the user's perspective
not sure. I don't think lava particles would work because it's not only lava in the pot. Maybe like ash / smoke particles, or do lava particles or water particles depending on what's in the pot?
Yeah I just wouldn't add this rn because we'll remove it soon anyway
👍
oh I'm just a buffoon, I was trying to get lava out with the wrong pipes mb
Really confused, what do you mean? |
I can make a small fallback map and add some exceptions like fire, but that isn't something I want to really think about maintain every time a similar situation happens
The idea was to have a variable tickInterval in order to not tick when unnecessary, however now, |
It's just adding items to a map when we find them, it's not really a high maintenance thing. That doesn't seem like a thing to worry about tbh
I don't understand what you're saying. What does 'variable tickInterval in order to not tick when unnecessary' or 'obtains the tickInterval once and always uses that one' mean?? can you give an example maybe? |
|
the method getTickInterval is called, and returns after how much time next tick is, normally it is saved on a constant variable inside the TickManager. in this case, possibly in others, the getTickInterval output isn't constant, in this case it depends on the block below the crucible. however the tickInterval used in the coroutine is set in stone, it can't be changed, even if it is supposed to be changed in order to tick faster, as maybe the player instead of using a torch below the crucible puts a lava bucket. Proposal: every time after the tick is processed, instead of using a saved constant value, call getTickInterval every time |
Oh I see. So you're just saying that |
No, but yes, that is also a consequence, i am not using setTickInterval at all in this example, i am directly overriding getTickInterval in this case |
|
Have you tried calling setTickInterval to set the new tick rate when needed? |
|
Ah wait I see. So in PylonTickingBlock 'val tickDelay = pylonBlock.tickInterval' is called before the loop. Yeah you can just change that to call getTickInterval each time and then it should work |
|
Requires pylonmc/pylon-core#521 |
LordIdra
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.
Just a couple more small things I noticed.
Another thing, the ticking rate seems to have gone up a LOT, like it chomps up cobblestone like crazy now. Assuming this isn't intended?
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/Crucible.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/recipes/CrucibleRecipe.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/machines/simple/Crucible.java
Outdated
Show resolved
Hide resolved
yes, it depends on the block below, if you use lava it is supposed to be a LOT faster, if you use a torch, it should have same behaviour as before (minimum speed) |
Seggan
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.
Played around with it ingame, ngl I would prefer you throwing in items instead of right clicking the crucible. Barring that, at least a way to get the items out pls?
| } | ||
|
|
||
| @NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
| public static final class HeatManager { |
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 is this class needed?
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.
I wanted to extract the whole Heat handling tbh, but I am not sure where
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.
I don't think this should be in the recipe type, as it's to do with the crucible, not the recipe type. I don't think this needs to be a whole separate class either, you can just have static fields / methods in Crucible - this is what we do with other blocks when we have some kind of registry associated with them
| animate(display, 0, duration, matrix); | ||
| } | ||
|
|
||
| public static @NotNull ItemStack makeItem(@NotNull NamespacedKey key) { |
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.
What do these functions do?
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.
... make items?
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.
Could we call this itemFromKey? it's not really obvious what 'make item' means at first glance
| @@ -0,0 +1,29 @@ | |||
| pylonbase:cobblestone_to_lava: | |||
| input-item: | |||
| minecraft:cobblestone: 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.
use #pyloncore:rocks instead
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.
LordIdra
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 should be able to put in items when it's not heated. Right now you can't. I think making this not a multiblock will fix that.
| import static io.github.pylonmc.pylon.base.util.BaseUtils.baseKey; | ||
|
|
||
| public final class Crucible extends PylonBlock implements PylonMultiblock, PylonInteractBlock, PylonFluidTank, PylonCauldron, PylonBreakHandler, PylonTickingBlock { | ||
| public final int CAPACITY = getSettings().getOrThrow("capacity", ConfigAdapter.INT); |
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.
Nit but should be capacity (same for smelt_time)
| public void clearInventory() { | ||
| this.processingType = null; | ||
| this.amount = 0; | ||
| } | ||
|
|
||
| public boolean isValid(ItemStack item) { | ||
| for(var entry : CrucibleRecipe.RECIPE_TYPE.getRecipes()) { | ||
| if (entry.matches(item)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
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 don't really seem like they should be helper methods as they're so simple. If you really want isValid, it should probably be a method on the recipe type instead
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.
clearInventory is meant in case someone wants to clear the inventory from code, i can move is valid
| if (recipe.matches(processingType)) { | ||
| if (!new PrePylonCraftEvent<>(CrucibleRecipe.RECIPE_TYPE, recipe, this, null).callEvent()) { | ||
| continue; | ||
| } |
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 ifs can be collapsed into one if
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.
keeping it like that for clarity and because everywhere else in the code has always been like this, so for consistency,also not really imporant as you wanted to remove those events no?
| @Override | ||
| public @NotNull Set<@NotNull ChunkPosition> getChunksOccupied() { | ||
| return Set.of(new ChunkPosition(getBlock())); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean checkFormed() { | ||
| return getHeatFactor() != null; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isPartOfMultiblock(@NotNull Block otherBlock) { | ||
| return otherBlock.equals(getBelow()); | ||
| } |
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.
I don't think this needs to be a multiblock. It'll be simpler that way and the performance impact is negligible. I know that other blocks like the mixing pot do this, but I have removed this in an open PR
| public @NotNull Block getBelow() { | ||
| return getBlock().getRelative(BlockFace.DOWN); | ||
| } |
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 doesn't really seem like it needs to be a helper method, there isn't really any benefit to not just inlining this afaict
|
|
||
| if (material.isItem()) { | ||
| return new ItemStack(material); | ||
| } else { |
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.
nit redundant else
| animate(display, 0, duration, matrix); | ||
| } | ||
|
|
||
| public static @NotNull ItemStack makeItem(@NotNull NamespacedKey key) { |
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.
I feel like I might have mentioned this already, but this seems like a common enough thing that it should be a core util. In fact, don't we already do something similar when deserializing recipe inputs? Presumably the logic from there could just be extracted
| crucible: | ||
| name: "Crucible" | ||
| lore: |- | ||
| <arrow> Place items in the crucible and <insn>right click</insn> to melt them |
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.
The lore is incorrect. You need to right click to put items inside the crucible and then it melts them
| pylonbase:crucible: | ||
| pattern: | ||
| - "C C" | ||
| - "C C" | ||
| - "CCC" | ||
| key: | ||
| C: minecraft:brick | ||
| result: pylonbase:crucible | ||
| category: building | ||
|
|
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.
Might it be worth making this refractory brick? It would make sense, add another use case for refractory brick, and also gate a potentially very powerful item (practically infinite lava) a bit further along
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.
wasn't the curcible mean for early game though
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.
Not specifically. I was thinking it might be for skyblock, but I think there are a number of recipes you might need to modify anyway for that and you can easily do that now that recipes are data driven
| new ParticleBuilder(Particle.SPLASH) | ||
| .count(20) | ||
| .location(getBlock().getLocation().toCenterLocation().add(0, 0.5, 0)) | ||
| .offset(0.3, 0, 0.3) | ||
| .spawn(); | ||
|
|
||
| new ParticleBuilder(Particle.CAMPFIRE_COSY_SMOKE) | ||
| .count(30) | ||
| .location(getBlock().getLocation().toCenterLocation()) | ||
| .extra(0.05) | ||
| .spawn(); |
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.
I think in terms of the particles you could just continually spawn ash + smoke particles every 10 ticks or so and that would look pretty good



Fixes #299