Start soulbinder #92
No reviewers
Labels
No labels
Area-Assets
Area-Backend
Area-Conduits
Area-Datapacks
Area-Lang
Area-Mod Compat
Area-Parity
Area-Rendering
Good first issue
MC-1.19.2
MC-1.20.1
MC-1.20.4
MC-1.20.6
MC-1.21
MC-1.21.1
Modtoberfest
P-0-High
P-1-Medium
P-2-Low
Status-Awaiting Response
Status-Behind-Flag
Status-Blocked
Status-Cannot Reproduce
Status-Duplicate
Status-Help Wanted
Status-Incomplete Report
Status-Invalid
Status-Needs LTS Backport
Status-Needs Updating
Status-Stale
Status-To Implement
Status-Triage
Status-Wontfix
Status-Wontmerge
Type-Backport
Type-Bug
Type-Documentation
Type-Enhancement
Type-Question
Type-RFC
Type-Suggestion
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Team-EnderIO/EnderIO#92
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/soulbinder"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Soul Binder and recipes
Description
Please write a summary of the changes you have implemented; explaining your design choices if necessary. Please also include a list of things to test if it isn't clear.
Please also disclose whether this is a breaking change.
Todo
Checklist:
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;use slotaccess
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;yep, move to config
filter to livingentity, as the spawning should not be stopped by fishingbobber, partentities or items
why should there be a limit for this?
should we allow exact copise? could result to dupes when entities hold shulkers or in general items
use instanceof cast
use instanceof cast
@ -0,0 +28,4 @@public void addCommonTooltips(ItemStack itemStack, @Nullable Player player, List<Component> tooltips) {itemStack.getCapability(EIOCapabilities.ENTITY_STORAGE).ifPresent(entityStorage -> entityStoragewould resolve and map calls work to prevent 2 nested ifPresents? if not, this is fine
@ -0,0 +1,31 @@package com.enderio.machines.common.menu;merge to one if statement
the tag used by cyclic is
forge:experience, we should use that as well@ -0,0 +1,105 @@package com.enderio.base.common.util;put 24 in a private static final field
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;There was one in the past, I guess to stop massive farms?
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;Hmm good point, maybe just getting the entitytype would be enough.
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;If we add a limit it should be discoverable in the gui. We could also reduce the efficiency, if too many spawners are there, so their cummulative speed would stay the same. So if there are 11 spawner, max is 10, there should be a 10/11 chance to succeed the spawn. Also make that shown in the gui.
that's my opinion, but we could also discuss this
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;maybe put it in a config, multiple copymodes like none, health and all and add a comment what each mode does
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;Make this an enum somewhere or how do you see it?
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;enum sounds reasonable
@ -0,0 +1,168 @@package com.enderio.machines.common.blockentity;Should this be configurable through the config like Vacuum Blocks ?
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;This capability is present in both the soul vial and broken spawner. The recipe only allows for soul vials in the first position
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;You can also add Powered Spawners in the first slot which shouldn't happen
@ -0,0 +1,105 @@package com.enderio.base.common.util;import com.enderio.core.common.util.Vector2i;either fix comment or fix number
rename to EXP_TO_FLUID
@ -0,0 +119,4 @@public static class Container extends RecipeWrapper {private final FluidTank fluidTank;You may want to swap this for a machineFluidTank, since you only want fluid xp to flow in. You can do both of those things automatically through that class ;)
@ -0,0 +28,4 @@public void addCommonTooltips(ItemStack itemStack, @Nullable Player player, List<Component> tooltips) {itemStack.getCapability(EIOCapabilities.ENTITY_STORAGE).ifPresent(entityStorage -> entityStorageI mean probably. Do you prefer it over this?
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;This is part of the design when I made it. It needs the first slot to have the capability for the recipe to work. It doesn't impose other restriction besides that, so yeah, powered spawners could also fit in it. I didn't want to limit the first slot to just soul vials, though maybe that's just simpler?
@ -0,0 +119,4 @@public static class Container extends RecipeWrapper {private final FluidTank fluidTank;This container is purely used for the recipe and is just a reference to the real tank. But you do raise a good point of having a second look at the real tank, as it might be replaced by machineFluidTank. It also seems to miss somethings right now, good catch.
Hmm this is actually more complex maybe. If the tank has one type of experience fluid, will it allow another to be "merged" into it? Should we make this happen? The safest is to only use our own experience though.
it would be the safest, but I think it should accept all of them
The thing is that it would need some special code and care, since it would need to ignore the (stack) fluid and still fill when it's not the same. And than we get to the issue of what to return inside the tank. And possible mods using different units...
@ -0,0 +28,4 @@public void addCommonTooltips(ItemStack itemStack, @Nullable Player player, List<Component> tooltips) {itemStack.getCapability(EIOCapabilities.ENTITY_STORAGE).ifPresent(entityStorage -> entityStoragemap and flatmap would probably look nicer , but it's not important and personal preference
mods using different amounts for the same thing is not the case I think. And if so a standard would be better, because why should the tag exist if it's not meant for equality
I'll give it a go, but I might need to rewrite some stuff. And it will need some good testing.
@ -0,0 +119,4 @@public static class Container extends RecipeWrapper {private final FluidTank fluidTank;machineFluidTankisn't a thing though, as far as I can see. We do have aMachineFluidHandleris that what you ment?@ -0,0 +1,168 @@package com.enderio.machines.common.blockentity;I'm planning on bringing all the range methods to the base machine class in a next PR. Should these colours be in a config, they might have been in the past, but does it have an advantage? Does it even help colour blind people?
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;The recipe checks for soul vial and so it probably shouldn't accept other items (since we don't a recipe validity notifier inside the screen). I can see people getting confused when items get accepted in slots but nothing happens.
@ -0,0 +1,168 @@package com.enderio.machines.common.blockentity;The config right now resides on the server side and thus not very useful as the colors will be synced to all players. It can be helpful but we need to move it client side for any use. If we do decide to not make it configurable, we will need to find a good default color that works for all
@ -0,0 +1,155 @@package com.enderio.machines.common.blockentity;I wanted to be future proof in case we did want a recipe that used something else. But now that I think about it, that's probably not gonna happen. I'll change it to only take soul vials, that's probably fine.
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;I'd say an efficiency cap is reasonable to avoid overcrowding; should definitely be configurable and displayed in a tooltip for clarity.
@ -0,0 +1,182 @@package com.enderio.machines.common.blockentity.task;Hmm, so simply make there be a chance for them to fail if to many are present? That should be fine, I can add it to the "blocked" enum.
Looking good, just leaving a few notes for you here :)
This can be fully removed now; if its necessary in future I'll put it back with the proper URL :)
@ -0,0 +1,168 @@package com.enderio.machines.common.blockentity;Just a stylistic point; maybe store the RL of the pig in a constant just so its abundantly clear (and we're also not creating new objects regularly)
Capitalisation of Mobs and Spawners is likely unnecessary here; but could be stylistic preference on my part.
@ -4,14 +4,9 @@ import net.minecraftforge.common.ForgeConfigSpec;The clarity here is nice because otherwise it'd be a random
dropChancekey with no context.I think this looks pretty good to go now, just need that model and we're golden! Nice work!
@ -0,0 +30,4 @@}@Overrideprotected void init() {maybe do that in renderLabels
@ -0,0 +130,4 @@public void shouldShowRange(Boolean show) {this.rangeVisible = show;}remove !isClientSide() check as that is already forced by ServerLevel
remove unused name parameter
@ -0,0 +116,4 @@public int getNeededXP() {return container.getNeededXP();}remove question mark and add the comment on the line above
@ -0,0 +116,4 @@public int getNeededXP() {return container.getNeededXP();}It was since I wasn't sure we should do it. But if you're fine with it, that's good.
@ -0,0 +116,4 @@public int getNeededXP() {return container.getNeededXP();}yeah, I'm fine with that approach
:shipit: