copygirl / BetterStorage

A Minecraft mod aimed at offering more storage options.
http://copy.mcft.net/mc/BetterStorage/
MIT License
57 stars 33 forks source link

Attachment Issue #284

Closed Speiger closed 9 years ago

Speiger commented 9 years ago

Your Lock API is working fine but if people want to add a Attachment system they have either: A: changing minecraft to make everything runnable with your mod because of ASM (which not every mod uses), and that could create problems, or B: create a Dead version of your mod which only provide the nessesary files that the system is at least for the compiler ok and then have to work with a dead mod in your workspace... that maybe even could break some stuff...

Would be kinda cool if you could make some changes that allow at least that people can register their/your attachments without changing the whole game.

copygirl commented 9 years ago

The lock API and attachment system is crap and I've already though of a better way to do it (using entities), though I'm not interested in putting the work into something of that scale. And the attachments weren't meant to be used outside of BetterStorage, though I'm not sure why you think ASM would be required to do that. While I'm still not sure what the proper way to reference / include another mod in your workspace is (it's just confusing me), I'm pretty sure it's possible, as I've done so myself.

Speiger commented 9 years ago

Using Entities is not really much better... Since they share their ticks with TileEntities, and butcher all registered entities distroy them.... TileEntities are already good. You just may have used more interfaces for them. Because you made maybe blocks lockable but never call them without IHasAttachment... Also maybe functions like readfrom/writeto nbt should be implemented into the Attachments class so that the nbt data is already done and people do not have to care about every single case. Also i could help you to make the TileEntity version of locks and co so good that you do not need to rewrite it ^^" Because its already good (for intermal useage) you only need to make it a little better... (There are some ways to code something that people can use it without trouble)

To ASM. There are people out that can not use ASM (me inclueded), and if you have a mod that uses ASM, that changed stuff, then they also changed the functions/methodes and made them accessable for themselves. Now what happens if someone has uses these functions but do not have the ASM classes installed that make them accessable? They will crash terrible, and because they are not able to use ASM they have to reinstall their workspace and find out which function/field did exsist for them and which not...

Thats why i made a dead version of your mod that allows me to assemble everything i need but your mod will not run in my workspace. In short i made your API bigger with a dead mod.

Note: Everything that you made with ASM could be done with Reflection (even so if it is slower it is fast enough for that what you did)

Speiger commented 9 years ago

Sorry missreaded the asm part.

You are using asm for the crafting station and because attachments require a full version of your mod i need asm for your crafting station...

^^" Well i could change it to reflection...

copygirl commented 9 years ago

Using lock entities has the following upsides:

I'm not willing to fiddle around with something as complicated as the attachment system anymore, and I wouldn't recommend using it either way. If you want to, you can have at it, but really, BetterStorage is dying (for me). I doubt anyone would use something like this anyway. You're the first to even mention it.

I don't use ASM, and I'm 95% sure BetterStorage doesn't either. It used to have some access modifiers in older versions (1.6 and before I think), but we've switched to using just reflection. Can you point me to where the crafting station (or anything, really) requires ASM to work?

Speiger commented 9 years ago

Its the Inventory that you are using. In your version it is a public ItemStack array at my source it is a private ItemStack array. I do not know why it is public or private or what is doing it but the only difference we have (at least in 1.6.4) is that you are using Forge Gradlew and i am using MCP (since it is for me the better option).

^^" Lets say i am not a fan of Entities because you can easily remove them from the world because it is only the function setDead that you have to call to distroy them from world which can be used for a lot of things^^...

And to your two upsides: You can do that even with TileEntities you know that because if you making an attachment then you also can use the same thing for it for interaction.

I am asking myself why you do not even going that rout already:

People can register their TileEntities for attachments and can say which type it is, like door/inventory/tank or something like that. And when they register their TileEntity they get a DataList which contains the AttachmentData with settings like: canAccess, shouldEmitRedstone, getOwner and even more what you think. and on the Other hand you can registerTheTileEntity in your DataList with a AttachmentList which you provide for any Attachment. And with Events you can detect BlockInteraction of any kind and do something with it. And extra data like facing is easily accessible via Interfaces that you say are required.

And with Storing data you simply use a File that you create and Chunk Loading/Unloading events. ^^"

Also i am not the only person who uses better storage. It has maybe some bugs but it works really well and has cool features (in both terms, playing and coding i learned a lot from your mod), and to tell at least 1 other person which is using it (maybe not the locking system but your mod) is CovertJaguar...

Speiger commented 9 years ago

And because i do not want to spam you with issus requests in github i post it here:

The Crafting station, why has it 9 slots as output? do they have any use? If not why don't you do a recipe missmatch helper with it. If a recipe has multible outputs that station would be perfect for it :) My own crafting station that is doing that is based on that idea which came from your Crafting Station^^"

copygirl commented 9 years ago

(at least in 1.6.4)

That's the problem. We've changed to just reflection in 1.7 an onward. The 1.6 version has tons of bugs and I'm not sure which, especially with the crafting station. Why are you still on 1.6?

And I'll still stand with my opinion that if I were to continue BetterStorage in its most likely form (which is being broken up into multiple mods), and I would actually redo the locking mechanics, I'd choose entities. Though that doesn't mean that it's the only good way to do it.

The Crafting station, why has it 9 slots as output? do they have any use?

The custom crafting recipes you can do with the crafting station are pretty powerful. For example I have a cardboard enchantment recipe, it uses multiple input cardboard items, one book and outputs the cardboard items + the enchantment, for some experience cost, without even using up the book. Either way, the 9 outputs slots are just what they are, 9 output slots, and they can be used for custom recipes (such as ones created using MineTweaker). There's already a mod which handles duplicate recipes by allowing you to pick the one you want in the regular crafting table (though not sure which one).

Speiger commented 9 years ago

Ok.

I am still on 1.6.4 because most stuff which in 1.7.10 happends is something which i do not like so i decide to stay on there. The only mod i support for 1.7.10 is IC2 classic. Also i want to finish my mod before i am thinking about updating because if i always update and update and update then i do not have time for adding the stuff i want... Here in 1.6.4 its extremly quiet i do not need to take care about modupdates/api updates because everything is set maybe even bugs but that is a thing that i do not care of.

Speiger commented 9 years ago

Anyway dropped the lock support because to hard to handle...