-
Notifications
You must be signed in to change notification settings - Fork 9
Add talismans #366
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?
Add talismans #366
Conversation
src/main/java/io/github/pylonmc/pylon/base/content/talismans/base/Talisman.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/BarteringTalisman.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/BarteringTalisman.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/EnchantingTalisman.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/HungerTalisman.java
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/base/PDCKeyTalisman.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Seggan <[email protected]>
…nchantingTalisman.java Co-authored-by: Seggan <[email protected]>
…into add-talismans
f5753c1 to
35d7f1b
Compare
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.
Nice work on this. Would be nice to have a category for these now that we have so many
Alternate item suggestions (w/ clay balls where appropriate):
- hunger talisman - golden carrot
- luck talisman - rabbit's foot
- enchanting talisman - enchanted book without the glint
- hunting talisman - skeleton skull
src/main/java/io/github/pylonmc/pylon/base/content/talismans/EnchantingTalisman.java
Show resolved
Hide resolved
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 talisman flat out does not seem to work; cannot get it to increase any enchantment levels by one, and choosing the first enchantment from the list with a full enchanting table causes the level to be decreased by one lol
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've gotten it to work. Problem is that since we're using the enchanting table seed for the rng seed of the talisman, it will only up the same enchantments over and over again, in my case sharpness 1 -> 2 and bane of arthropods 1 -> 2. Fixed by assigning each item that comes through an enchanting table with the talisman a uuid. The exploit I could see wrt to that is that someone nefarious could run thousands of items through the enchanting table and balloon the file size of the world. Although at 16 bytes/uuid, it would take 67,108,864 items to fill up 1GB of space which is a lot so I'm not insanely worried. It's still not perfect though, if there are 3 sharpness 1s in the same view, they will all have their levels increased by 1. This is because the chance has to be deterministic from the etable, item id, and enchant type in order for us to be able to apply the enchant in the EnchantItemEvent since we don't have access to the EnchantmentOffer order.
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.
There are much easier ways to inflate file size so I would not worry about that lol. I don't quite understand what's going on here but happy as long as it works
src/main/java/io/github/pylonmc/pylon/base/content/talismans/FarmerTalisman.java
Show resolved
Hide resolved
src/main/java/io/github/pylonmc/pylon/base/content/talismans/HungerTalisman.java
Show resolved
Hide resolved
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.
Have killed a bunch of creepers/zombies/etc and this does not appear to be working. Also probably needs a buff
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.
uh idk what this is on about, like my first pig I killed I got like 4 porkchop when max is 2 w/o looting. Tried with zombies too and was getting multiple rotten flesh
|
(Let me know when this is ready for re-review - not sure if it is rn) |
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.
Sorry I didn't review earlier, I kept forgetting... but here it is
| import java.util.concurrent.ThreadLocalRandom; | ||
|
|
||
| public class BarteringTalisman extends PDCKeyTalisman<Float, Float> { | ||
| public static final NamespacedKey BARTERING_TALISMAN_KEY = new NamespacedKey(PylonBase.getInstance(), "bartering_talisman"); |
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 FYI - you can use BaseUtils.baseKey("bartering_talisman") here. Won't comment on the other instances; up to you if you change this, it's not a big deal
| } | ||
| }; | ||
|
|
||
| public static final SimpleStaticGuidePage TALISMANS = new SimpleStaticGuidePage(baseKey("talismans"), Material.TOTEM_OF_UNDYING); |
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 page is missing a translation key
|
|
||
| PylonGuide.getRootPage().addPage(BUILDING); | ||
| PylonGuide.getRootPage().addPage(CREATIVE_ITEMS); | ||
| PylonGuide.getRootPage().addPage(TALISMANS); |
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.
Would make more sense to have this after 'combat' in the guide
| } | ||
|
|
||
| public static final ItemStack HUNGER_TALISMAN_SIMPLE = ItemStackBuilder.pylon(Material.CLAY_BALL, BaseKeys.HUNGER_TALISMAN_SIMPLE) | ||
| .set(DataComponentTypes.ITEM_MODEL, Material.CLAY_BALL.getDefaultData(DataComponentTypes.ITEM_MODEL)) |
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.
Maybe a better item would be cake, pumpkin pie, golden apple, or something? Clay ball doesn't feel right
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 little convention thing, but we don't really use base folders - I'd suggest just moving the folders in content/base to content/talismans
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.
General UI thing, might be better to separate out the whole 'while your inventory' part, maybe something like
<arrow> Gain saturation and hunger every %period%
<arrow> Only works while in your inventory
Some of the wording is really awkward as it stands ('Expands your lungs so you can breathe longer underwater while in your inventory')
| hunger_talisman_simple: | ||
| name: "Simple Hunger Talisman" | ||
| lore: |- | ||
| <arrow> Gain saturation and hunger every %period% while in your inventory |
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.
'Gain' -> 'Increases your'
| farmer_talisman_simple: | ||
| name: "Simple Farming Talisman" | ||
| lore: |- | ||
| <arrow> Gain a small chance to get an extra crop drop while in your inventory |
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.
Maybe reword to 'Gives you a chance to get an extra drop when breaking a crop' for slightly better clarity
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.
Another little UI thing: maybe instead of having stuff like 'Change for extra item', 'Chance to not consume gold' etc you could just write 'Success rate'. Would be a bit more clear and consistent and might allow you to simplify some of the talisman descriptions too as you wouldn't need to mention 'Gain a chance to...'
| luck_talisman_advanced: | ||
| name: "Advanced Luck Talisman" | ||
| lore: |- | ||
| <arrow> Makes you a much luckier ducky while in your inventory |
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.
Needs clarification on what the heck this means in practice



Implements the following:
Makes tick interval of talismans configurable. Leaving Talisman ring #335 for future pr since with the Talisman class, this likely won't need to be refactored much to accommodate that.