-
Notifications
You must be signed in to change notification settings - Fork 46
Added new item ArcaneCraftingUpgrade #155
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
|
@OneEyeMaker can you have a look ? |
Nikolay-Sitnikov
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 seems ill-considered, so far.
| if (playerMP != null) return playerMP | ||
| } | ||
| world.playerEntities.asScala.collectFirst { | ||
| case p: EntityPlayer if !p.getCommandSenderName.toLowerCase.contains("[oc]") => p |
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.
Grabbing any non-OC player on the server, including fake players & possibly incompatible players (like Salis Arcana's non-EntityPlayerMP player, which owns every research?) and using that for checking if the player owns the research? Brave move.
Funnily, this means you can use someone else's research & vis discounts by just logging off.
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.
Yea my bad, I wasn't really sure how to implement it.
|
|
||
| def craft(wantedCount: Int): Seq[_] = { | ||
| var player = host.player | ||
| val wandSlotIndex = 12 // Change this if your wand is in a different slot |
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.
Who is supposed to change this, and how?
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 was setting open to change, before implementing.
| } | ||
|
|
||
| countCrafted += result.stackSize | ||
| FMLCommonHandler.instance.firePlayerCraftingEvent(real_player, result, this) |
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.
Oh, how nice, you're giving some player (possibly not even the owning player) warp for crafting the item. You're lucky that Salis Arcana fixes the crash that would happen if a fake player got warp added to them.
|
Also, doesn't this make OpenComputers have a hard dependency on Thaumcraft? I don't see any code to disable the item if Thaumcraft isn't installed. |
|
@AX3Lino @Nikolay-Sitnikov any news on this PR? |
|
@boubou19 I'm converting this to draft for now, since I haven't heard much progress or justification for this change. I am not excluding the possibility that this may continue development, but I don't want it clogging up the review queue. |
I made new item ArcaneCraftingUpgrade, that is tier higher than CraftingUpgrade. This upgrade does vanilla crafting and also arcane crafting (Thaumcraft). Made to broaden automatization possibilities for GTNH. To do the arcane craft wand/scepter need to be in robot`s inventory.
Tips:
research check and vis discount is based on player who created/put down the robot
vis discount is calculated based on what is player
s wearing, better idea would be to calculate using armor in robots inventory (not yet coded)wand slot is leftmost slot under crafting slots in robot`s inventory (in lua slot 13)
Known issues:
rendering item on the back of robot is not ideal
recipe for this item does not show in nei, also it is open to change (it is defined just to work in user.recipes )
tooltip is same as CraftingUpgrade