jaminv

I have a multi-block machine that has connected textures. When the machine is active, the main machine block emits light. When it does so, it creates a hard light line between itself and connected blocks. Is there anything I can do about this? (Version 1.12)

I see what you mean now. While this did run properly on the dedicated server (due to layers on abstraction), when I'm doing everything properly it just isn't necessary. So much easier to just put the client code in a client proxy than having to worry about burying client code and/or accidentally referencing client objects in server code. --- Man I wish the tutorials I found had told me that I could test the dedicated server using the run configurations that are automatically setup in Eclipse (and that I should do so regularly). I had to go specifically looking for that information. I ended up having a fluid that I missed an IHadModel on and the registerModels() function referenced IStateMapped directly in the Block class. I also had some client message handlers that referenced Minecraft.getMinecraft().world and needed to be registered in a client proxy as a result. I also discovered that JsonUtils.isString() is (for some unknown reason) client only. I've got everything cleaned up now. I wonder if "test you code often on the dedicated server, here's how" should be in the common issues and recommendations thread? --- Thanks for all the help. Turns out that it's just another way of thinking about it, but it's still possible to consolidate code without using IHasModel interfaces.

So optimally: In the RegistryEvent, call something like BlockInit.init(), which handle the instantiation, registration, and cache the model information? EDIT: I guess there's no real reason to break off into another class.

I could just as easily instantiate them to an array and then registerAll() the array in the RegistryEvent. What's the best place to instantiate them? PreInit? Does that happen before the RegistryEvent?

That's basically what I'm doing now, although I didn't realize I could getValidStates(). That might help with some abstraction. Ok, yes. That's almost exactly what I'm saying. --- https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/farming/FarmingBase.java#L103-L137 It appears that you're not using the RegistryEvents to register your blocks/items? Is that not required? I feel like a lot of the reason IHasModel ended up existing in the first place is to avoid disjointed code: where you've got instantiation in one place, registration in another, model registration in another, and then the block code somewhere else. So the tutorials I followed had a "instantiate the object, then have the instantiated object handle all its own registries" [anti]pattern. It appears you just handle instantiation, registry, and (deferred) model registry in one place. That seems like a pretty good pattern. I think that's the missing piece I was looking for.

Well, the simple one is something like an `BlockOre` block where the ore type is stored in meta data. Instead of registering one model, I have historically iterated through the EnumOreType and called registerVariantRenderer() for each. Although thinking about it, I suppose I could just use a blockstate JSON file for this (as I do for the block itself). A more complex example is a series of blocks that use IBakedModel rendering and uses metadata for rendering. I actually need a different IBakedModel for each metadata state, otherwise the block's particle texture will be the same. Therefore, I need to create a CustomStateMapper that iterates through the EnumMetadata and provide a separate ResourceLocation for each. I'm not sure there's a way around this one, (perhaps I'm missing something?) and I'd really like to put this code with the objects themselves. This is specialized to the BakedModelProvider interface, so perhaps I could just add another method to the interface that returns an IStateMapper and defaults to a default implementation? Since I only currently need one VariantStateMapper to handle several similar block types, there wouldn't be any duplicate code.

1) Some models require additional consideration. I'm asking if IHasCustomModel is an acceptable pattern when a block requires extra code to handle its models. The data required to build the models is indeed public, but it may need special information like a Enum specific to that block. It makes more sense (to me) to localize that code in the same package as the block itself rather than push it into the registry packages. 2) BakedModelProvider is only referenced in my ModelRegistryEvent (and there only by instanceof), and in my BakedModelLoader, which is only registered in the ClientProxy. Additional, the only method of interface is specified as SideOnly (Side.CLIENT). I've actually modelled it after CodeChickenLib's IBakeryProvider (except my implementation is vastly simpler). Not saying that just because someone else did it makes it correct, but it seems like a lot of consideration has been applied here. (also the only method does nothing more than return an object, the `bakeModel` method has to be run on that object before any client-side code is referenced, and that is only done after BakedModelLoader provides a model to the client and bake() is run on that model). It appears that the ModelRegistryEvent is only ever run on the client side. Is that correct?

Not sure if this still applies to 1.14, but it probably does. In your block class you need to override the isOpaqueCube method and return false: @Override public boolean isOpaqueCube(IBlockState state) { return false; }

I followed a tutorial sometime ago, and of course I ended up with an IHasModel interface buried in my code. At the time, it kind of made sense because it wasn't really as simple as "of course everything has a model". Some blocks actually had custom item variants, so they would provide a different registerModels() implementation that handled the custom item variants. At the time, it felt like a pretty good use of abstraction. Except that in order to avoid a bunch of duplicate code, every Block derives from a `BlockBase` class that provides little more than a default registerModels() implementation... which is definitely *not* a good use of inheritance. Now, my situation has become a great deal more complex. I have some blocks that have custom state mappers for customer model baking. Currently, these utilize a `BakedModelProvider` interface because the block needs to provide data to the BakedModelLoader. I'm not sure if this is just a `IHasModel` by another name. There's no actual `registerModels()` method to the interface. Because they are all handled the same, I've put the following code in my ModelRegistryEvent: if (block instanceof BakedModelProvider) { ModelLoader.setCustomStateMapper(block, new CustomStateMapper(new ModelResourceLocation(block.getRegistryName(), "normal"))); BakedModelLoader.register(block.getRegistryName().getResourcePath()); } Naturally, things have gotten even more complex than this as my default `CustomStateMapper` implementation isn't going to work for every model, so I may actually need to add a `registeryModels() ` method to my `BakedModelProvider` interface. --- I'm not sure exactly what about the IHasModel makes it an anti-pattern. From my experience, it seems that the forced inheritance (or duplicate code) required to make the abstraction work is the main culprit. If that's the case, then an `IHasCustomModel` class would still be a proper pattern, whereas an `IHasModel` would not. Is that so? Or is there something else that makes it an anti-pattern?

I've done a few network messages already, but every example I've found entails sending the network message to a TileEntity (or player). But some of my tile entity classes are getting really big and I'm starting to work on composition over inheritance and breaking out some of the behaviors into encapsulating classes that manage those behaviors. The problem that I have is that some of them rely on network messages. As far as I can tell, I don't think there is a way to send a message to a composition object without passing it through the tile entity first. I was wondering if anyone had any thoughts on best practices for handling this? There are two ways that I can think of: 1) the tile entity manages the state, sends and receives the message, and implements an interface that allows the composition object to request the state from the tile entity, or 2) the composition object manages the state, sends the message and the tile entity receives the message and passes it directly to the composition object. The latter means less code in the tile entity, but doesn't feel very encapsulated. Since there's no requirement that the tile entity pass the message along, it's possible for that trust relationship to be broken. I'm leaning towards the former, but maybe there's something I'm missing? A few concrete examples: I'm moving the machine processing loop (like a furnace) out of my tile entity. There are two network messages involved in this. The first is the processing state. Since the processing loop doesn't occur on the client (its just skipped), the server machine has to tell the client that it's processing. This information is used to change model textures. The second network message is the redstone option (ignore redstone, active on signal, active without signal). This is selected by the dialog on the client and then has to be passed to the server, which actually uses the information. Thoughts?

Where it says "Looking at:" (third line of text from the bottom), those are the coordinates for your furnace. X=168, Y=69, Z=239. That is not within the range X=[-10,10], Y=[-10,10], Z=[-10,10]. You probably want to use the code I posted that deals in relative coordinates, but I can't hold your hand through explaining how the MC coordinate system works.

I think you might be thinking in relative coordinates, but world.getTileEntity() is in absolute coordinate. If you wanted relative coordinates, you'd need to feed in a reference BlockPos into the function: public static List<TileEntity> getTileEntityInAABB(World world, BlockPos relativeTo, int rangeX, int rangeY, int rangeZ) { List<TileEntity> telist = new ArrayList<TileEntity>(); for (double y = relativeTo.getY() - rangeY; y <= relativeTo.getY() + rangeY; ++y) { for (double x = relativeTo.getX() - rangeX; x <= relativeTo.getX() + rangeX; ++x) { for (double z = relativeTo.getZ() - rangeZ; z < relativeTo.getZ() + rangeZ; ++z) { TileEntity te = world.getTileEntity(new BlockPos(x, y, z)); if (te != null) { telist.add(te); } } } } return telist; }

Are you sure your Y value is between -10 and 10? Y=-10 doesn't actually make any sense. If you're still having trouble, open the F3 screen and point at your furnace and post a screenshot.

Change the for() loops to "y <= maxY", "x <= maxX", "z <= maxZ". I'm not sure why IF has it exclude the max values. If your furnace was at X=10, Y=10, or Z=10, it would have been excluded. Besides that, I don't see anything wrong there. Maybe narrow it down to the exact block coordinates of the furnace and trace through it?

Yes. Anything that has an inventory has to be a tile entity. Make sure you've got your range right. Make sure minX is less than maxX, minY is less than maxY, and minZ is less than maxZ, otherwise one of the loops will end without actually ever starting. That's the advantage of having a bounding box class. You can just feed it two BlockPos objects and have it sort out which of the X values is min or max, etc.

Oh, you said "entities", so I thought that's what you meant. Scanning for tile entities is easy. Just loop through every possible x, y, z coordinate in range and call world.getTileEntity(BlockPos(x, y, z)), which will just return NULL if there isn't a tile entity for that block. Here's the loop that Industrial Foregoing uses: (https://github.com/Buuz135/Industrial-Foregoing/blob/master/src/main/java/com/buuz135/industrial/utils/BlockUtils.java) public static List<BlockPos> getBlockPosInAABB(AxisAlignedBB axisAlignedBB) { List<BlockPos> blocks = new ArrayList<BlockPos>(); for (double y = axisAlignedBB.minY; y < axisAlignedBB.maxY; ++y) { for (double x = axisAlignedBB.minX; x < axisAlignedBB.maxX; ++x) { for (double z = axisAlignedBB.minZ; z < axisAlignedBB.maxZ; ++z) { blocks.add(new BlockPos(x, y, z)); } } } return blocks; } This method just builds a List of BlockPos for every possible (x,y,z) coordinate in range, but you could easily alter it to something like this: public static List<TileEntity> getTileEntityInAABB(World world, int minX, int maxX, int minY, int maxY, int minZ, int maxZ) { List<TileEntity> telist = new ArrayList<TileEntity>(); for (double y = minY; y < maxY; ++y) { for (double x = minX; x < maxX; ++x) { for (double z = minZ; z < maxZ; ++z) { TileEntity te = world.getTileEntity(new BlockPos(x, y, z)); if (te != null) { telist.add(te); } } } } return telist; } I've simplified the parameters to minX, maxX, etc, but you'd probably want to use some sort of bounding box data structure like IF does. I also free-handed this code, so there may be syntax errors, but the premise is all there.

Well, Industrial Foregoing is open source: https://github.com/Buuz135/Industrial-Foregoing. I'm sure that mod does a lot of block/entity scanning. It seems like it'd be a pretty good reference. After just a quick glance, it looks like they're using world.getEntitiesWithinAABB(): https://github.com/Buuz135/Industrial-Foregoing/blob/master/src/main/java/com/buuz135/industrial/tile/mob/MobDetectorTile.java

AnimeFan8888 is right about returning ItemStacks. That would definitely cause an issue like this. You should have getWorkbenchResult() return like this: return ((ItemStack)ent.getValue()).copy(); There's a pretty good chance that will solve your problem. It certainly can't hurt. --- I don't know if it helps because it might be total information overload, but the recipe management system for my mod is here: https://github.com/jaminvanderberg/AdvancedMachines/tree/master/src/main/java/jaminv/advancedmachines/util/recipe. It includes a three-ingredient recipe manager that handles ingredients in any order, ore dictionary, etc. There's a lot there because recipe management can get quite complicated. But if it helps, great.

That is strange. It's even more strange that it the count is 1 at first and then changes it to 2. Since the update() routine is never performed on the client, it's likely not a client/server desync at the tile entity. It seems like it's happening somewhere in the container, but I don't see it. Unfortunately, I don't have time to debug through all of your code and I can't really run it yourself. You should trace through it and watch it happening step by step. I suggest subclassing ItemStackHandler and override the onContentsChanged. You can even narrow it down by creating an if (slot == 2) {} statement with a dummy operation in it, and add a breakpoint there. When you hit the breakpoint, you can look at the stack trace and see everything that it causing an update to the slot. @Override protected void onContentsChanged(int slot) { if (slot == 2) { int a = 0; // Breakpoint here } }

One way you could speed up ticks would be to scan the AOE for TileEntities that implement ITickable and then manually call update(). Since update() is also being called by the game tick clock, any additional time you called it would speed up any machines in the area. Obviously, this is basically the definition of a lag machine, so make sure any loop code you add is as lean as possible. I have no idea how you'd slow down tick clocks. You could easily do it for TileEntities that you control by simply skipping update() ticks based on a state. I don't know if there's a way to do it in a general-purpose fashion. I know that there are items in mods that speed up ticks. If their code is public, you could certainly scan through their code and see how they do it. I don't know if there's any objects the slow down ticks. If you know of one, see if you can find code for it. If not, it may not be possible.

In WorkbenchRecipes: private final Table<ItemStack, ItemStack, ItemStack> workingList = HashBasedTable.<ItemStack, ItemStack, ItemStack>create(); ItemStack doesn't have a hashcode() method, so it's just using the default. There's no real way to guarantee that it will arrive at the same hash value for similar stacks, especially when you factor in item counts, NBT, etc. I'm not really sure how this is working for you at all (unless you're only putting 1 of each item in at a time), but it will almost certainly cause you problems down the line. What I've had to do is create a wrapper class that implements hashcode() in a consistent manner: @Override public int hashCode() { // Although it might cause hash collision, we don't hash meta or NBT data. Hash collision should be fairly minimal. // We don't hash meta because wildcard meta data wouldn't work. // We don't hash NBT because then items with simple NBT data (like renaming in an anvil) would be rejected by the recipe. return item.getRegistryName().hashCode(); }

One thing I noticed is that you shouldn't be calling BlockWorkbench.setState() from your TileEntity. Instead, you should add a method called getActualState() from your Block. In that method you can call world.getTileEntity(pos), get data from the TE, and return it in an IBlockState. Here's how I've done it in the past. @Override public IBlockState getActualState(IBlockState state, IBlockAccess worldIn, BlockPos pos) { TileEntity tileentity = BlockHelper.getTileEntity(worldIn, pos); EnumFacing facing = EnumFacing.NORTH; boolean active = false; if (tileentity instanceof TileEntityMachineBase) { TileEntityMachineBase te = (TileEntityMachineBase)tileentity; facing = te.getFacing(); active = te.isProcessing(); } return state.withProperty(FACING, facing).withProperty(ACTIVE, active); } I don't think this is causing your current problem, it's just the correct way to do things. I don't think you should really be calling World.setBlockState() the way that you are. It could cause you problems down the line. The Block class uses a singleton pattern. Minecraft only creates one Block instance no matter how many blocks of that type there are in the world. So Block classes are basically already de facto static. You should never store anything in a Block class, but instead use block states and tile entities, which you are doing correctly. TileEntities are not singleton. Minecraft creates a separate TileEntity for every instance of a block in the [loaded] world. I don't see you doing anything wrong here. It's just important as a modder to understand what Minecraft is doing here. private WorkstationEnergyStorage storage = new WorkstationEnergyStorage(6000); ... private int cookTime, energy = storage.getEnergyStored(); You've got some weirdness with your energy handling. You probably shouldn't be storing the energy value in your TileEntity, but instead relying on WorkstationEnergyStorage storage to handle it for you. I'm not even sure how your machine actually works, because it doesn't look like `energy` should ever be more than 0. The above code is only called once, and it storage.getEnergyStored() will always be 0 when it's called. I don't see anything else syncing your `energy` value to the energy storage. Even if there is some code I'm not seeing somewhere, it's going to cause you problems down the line. There's no reason to store the energy value as an integer in your TileEntity. It's stored in your energy storage. Use that. ItemStack output = WorkbenchRecipes.getInstance().getWorkbenchResult(inputs[0], inputs[1]); if(!output.isEmpty()) { output = WorkbenchRecipes.getInstance().getWorkbenchResult(inputs[0], inputs[1]); smelting = output; cookTime++; energy--; BlockWorkbench.setState(true, world, pos); if(cookTime == 200) { cookTime = 0; output = WorkbenchRecipes.getInstance().getWorkbenchResult(inputs[0], inputs[1]); smelting = output; inputs[0].shrink(1); inputs[1].shrink(1); handler.setStackInSlot(0, inputs[0]); handler.setStackInSlot(1, inputs[1]); if(handler.getStackInSlot(2).getCount() > 0) { handler.getStackInSlot(2).grow(1); } else { handler.insertItem(2, smelting, false); } smelting = null; output = null; cookTime = 0; System.out.println("finish"); this.markDirty(); return; } } You've got some weird duplicate code in your update() method. You're initializing `output` 3 times to the exact same value. You're just wasting processor cycles. `smelting` is initialized twice but never used. `output` is set to null at the end of your code, but it's a local variable and this isn't necessary. Perhaps you were just trying different things out, but I would clean this up. It makes it easier to see what going on. Although I've spotted a few potential issues that might cause you problems down the road, nothing really stands out as the actual problem. Are you sure you've pushed your most recent code to Github? With the energy issue, I'm not sure how you get to the point where this is actually outputing items. Perhaps I'm looking at an older version of the code?

You're probably using a static variable somewhere. The TileEntities for each block should be all be different instances of your TileEntity class. I can't think of any reason why that might be so. But if one of the variables inside your TileEntity was declared as `static` then every TileEntity would share that same state. From the way you describe it, I'd guess that more than one of your variables is declared static.

Looking into Thermal Expansion further, it looks like what they do is pretty close to what I was saying. They pass the NBT data in the "onBlockPlacedBy" method of the Block (rather than passing it to the constructor). The TileEntity constructor sets the default energy capacity to an arbitrarily large value, which is then scaled down once onBlockPlacedBy is called or after the NBT is loaded. So I guess for my current specific requirements, there's no real reason to restructure my block metadata. I guess TE does it this way to reduce block ids and/or prevent the need for a bunch of different block classes?

I have a machine where I'd like things like energy capacity to be based on the meta data of the block. The meta data represents different stages of the machine. I'm having trouble delivering that data to the tile entity in a consistent way. Obviously, I shouldn't pass it to the tile entity in a constructor. Doing so doesn't work anyways because only the default constructor is called with the TileEntity is loaded from NBT. And the World member variable isn't available in the constructor when loading from NBT, so I can't poll that to get the metadata for the block. I can store the meta upon creation and save/load it from NBT, but there's no guarantee that the "meta" NBT will be read before the "energy" NBT. If the energy is loaded first, it would be limited to the lowest capacity and energy may be lost. I could initialize it to the highest meta, but it all starts feeling pretty "hack"y at this point... Looking it over, it appears the Thermal Expansion solves this issue by storing the machine type in the meta, and the machine stage in item NBT. I'm not entirely sure how this gets passed to the TileEntity, but I'm sure I can figure it out if I went down this route. But it's a pretty big code restructure. Is this indeed the best way to handle this? Or is there something I'm missing? It seems like it should be simple to get the block metadata in the TileEntity, but it's proving quite difficult. I've used it in some other places by just getting the block state, but that was always with fully initialized blocks and TEs. The difficulty I'm having is because I want to initialize TE data (like energy and fluid capacities) based on the metadata.