Skip to content
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

Expanded Impulse Cannon Augmentation #397

Draft
wants to merge 6 commits into
base: staging
Choose a base branch
from

Conversation

M-W-K
Copy link

@M-W-K M-W-K commented Jan 11, 2025

Draft PR for code that implements and tweaks content described in #393
Associated tasks:

  • Get the impulse cannon (augment) related API to a state compatible with most of the changes needed.
  • Implement augments that were more or less okayed during the discussion in [Feature] Expand Impulse Cannon Augmentation #393 and their research
  • Implement conversions that were okayed during the discussion in [Feature] Expand Impulse Cannon Augmentation #393 and their research
  • Determine how compatibility with gauntlet augments should be tackled and implement it
  • TEXTURES
  • Cannon models for new conversions (assuming I was right about the fact that different conversions result in different models, the code suggested that was the case but I haven't actually gone and stared at the ingame model to check)
  • More augments/conversions?
  • Put copyright notice in the header of new code files

Also of note is that this branch contains a commit that (very hackily) moves over to the RetroFuturaGradle build system, it's the only build system I've found for 1.12 that gives me functioning natives to launch the game in dev on my machine. This should not be merged to the main repo, and once this branch is in a finished state I will revert the commit with these changes.

@TheCodex6824
Copy link
Owner

TheCodex6824 commented Jan 12, 2025

Regarding the RFG change: unfortunately you will need to revert that when you're done. I develop on an "exotic" platform (ppc64le) and the RFG build.gradle fails when it tries to download an Azul JDK because my platform is unsupported. We also still target Java 8, so the modern java syntax adjustments enabled by RFG will either have to be ported to the normal build.gradle or reverted.

If there's something I can do to make the buildscript more compatible I would be happy to do it - but if RFG automatically downloads arm64 natives or something then I don't have that functionality at the moment (I just put my own in a natives folder in the repo root - you can see some parts of that in the normal build.gradle).

* Must be registered as a variant of the cannon item using
* {@link net.minecraftforge.client.model.ModelLoader#registerItemVariants(Item, ResourceLocation...)}
*/
default @NotNull ModelResourceLocation getLensModel() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget if ModelResourceLocation is a SideOnly(CLIENT) class or not - if it is, then this will have to either be changed or a new client-only interface has to be made. I suspect this might be why I originally had an enum instead of returning the model directly.

import net.minecraft.item.Item;
import net.minecraft.item.ItemStack;
import net.minecraft.util.ResourceLocation;
import org.jetbrains.annotations.NotNull;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we currently have these in the normal build.gradle, although I think just adding it as a dependency should work fine. I also have @ParametersAreNonnullByDefault set on the API top-level package-info.java, but from the name I guess that doesn't extend to return values.

}

@Override
@Nullable
public ICapabilityProvider initCapabilities(ItemStack stack, @Nullable NBTTagCompound nbt) {
SimpleCapabilityProviderNoSave<IAugment> provider =
new SimpleCapabilityProviderNoSave<>(createAugmentForStack(stack), CapabilityAugment.AUGMENT);
new SimpleCapabilityProviderNoSave<>(augments[stack.getMetadata()], CapabilityAugment.AUGMENT);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will put the same augment instance into every stack of the same type, which is fine now but would be a problem if the augments ever started to contain state. Unless these augment instances are extremely expensive to create, I think it'd be better to just make new ones to avoid leaving the shared instance footgun here.

@M-W-K
Copy link
Author

M-W-K commented Jan 13, 2025

As an opening, how were the sounds for the railgun/burst fire created/obtained? The charge rifle (I came up with a fitting name/lore, the Mirror Recursion conversion kit, a linked pair of impulse mirrors facing each other) will probably need a unique sound, and for simplicity probably have the pitch/volume changed based on charge factor.

As for your comments, yes the RFG stuff should never be pushed to main repo. I totally forgot to check that ModelResourceLocation was not clientside only when I made that change, but it appears to exist serverside. I will run a server test soonish and change this to an alternative if it causes a crash. The changes in the dependency file and other top-level things are also consequences of moving to RFG and will be reverted. Maybe the practices of RFG can be learned from, but from my end I only switched over to RFG so I could get working natives.

For the same augment instance issue, I actually want to discourage attempting to store data in the augment instance in the future. I'm not sure if caps can be lost when itemstacks are copied, and just in general storage of information should be handled by writing and reading NBT to the augment and cannon stack and/or creating a cache of [ user entity -> stored data ]. I should probably clarify this by making every method in the augment interface require passing in the two stacks in question.

To close out, I have two more augment/conversion ideas, one much more esoteric than the other.
The reasonable one is conceptually the idea of applying vulnerability stacks; this could take the form of either an augment or a conversion.
As an augment it would be a bit more limited: as an entity is damaged repeatedly without either missing or hitting a different entity, gradually ramping bonus damage would be applied. This is more limited because other augments modifying the damage composition of the cannon would not be compensated for, though base damage modifiers would be.
As a conversion it would be basically the same (taking into effect damage composition modifiers), but would overlap with the standard Beam quite a bit. It could be phrased as a direct upgrade/sidegrade of the normal beam.

The more out-there conversion is "True Void". This would be very expensive/hidden to reach, would be incompatible with all other augments, and cost the full 1000 impetus in the cannon to fire. The result would be equivalent to running /kill on an entity, or running /setblock air on a block, within touching distance. It would be framed as the natural result of combining and improving on the hyperion amplifier + primordial purifier combo. The issue being the concept's innate brokenness (if this is approved, I will add a config option to entirely disable it from operating.)

@TheCodex6824
Copy link
Owner

I finished looking through what you have so far, and it looks pretty good. I really like your research entries too, I think they are interesting and add extra lore to some things that really need it.

As an opening, how were the sounds for the railgun/burst fire created/obtained? The charge rifle (I came up with a fitting name/lore, the Mirror Recursion conversion kit, a linked pair of impulse mirrors facing each other) will probably need a unique sound, and for simplicity probably have the pitch/volume changed based on charge factor.

They were created by the talented @TechnoMysterio. I think making new ones would be no problem, and pitch / volume can be controlled in the code when playing the sound (although if you want to vary it within the same instance of a playing sound over time, that might require some custom sound code, but still definitely possible).

For the same augment instance issue, I actually want to discourage attempting to store data in the augment instance in the future. I'm not sure if caps can be lost when itemstacks are copied, and just in general storage of information should be handled by writing and reading NBT to the augment and cannon stack and/or creating a cache of [ user entity -> stored data ]. I should probably clarify this by making every method in the augment interface require passing in the two stacks in question.

Interesting, have you found issues with storing data in ItemStack capabilities in the past? We currently make heavy use of capabilities, as they store things like the Impetus charge level of an item for example. I did have to add special handling for syncing capability data to the client as it was (is?) an unsolved problem in Forge. You can see it as the getNBTShareTag / readNBTShareTag pairs in some items like this one. They also are able to store arbitrary data (as they're a class) instead of having to serialize everything to NBT. Of course, it did take a while to get them to point where they "just work" and don't lose data in some special situations (creative mode, leaving the end, etc...).

The reasonable one is conceptually the idea of applying vulnerability stacks; this could take the form of either an augment or a conversion. <...>

This sounds similar to the frenzy casting augment component. I think that can work, although the Impulse Cannon does enough damage where you have to be a bit more careful that the damage doesn't get out of control.

The more out-there conversion is "True Void". <...>

I am extremely hesitant to add a gameplay feature that makes use of void damage itself. I see void damage as a last resort to clean up entities falling into the void and in emergency server cleanup situations, and I really do not want to normalize mods blocking that damage for their super powered mob or whatever because someone was able to shoot it and kill it. If you add a custom damage source that does lots of damage, ignores armor, etc, that would be better, but I still don't know if I like having an item capable of oneshotting everything. Both for multiplayer reasons (getting one shot from hundreds of blocks away is not fun), but also for singleplayer balance. For example, making challenging but fair boss fights is already hard because of how good the Void Robes are, and often relies heavily on magic damage and endurance fights. I'll think about this one a bit more.

@M-W-K
Copy link
Author

M-W-K commented Jan 16, 2025

Alright, I can agree with the reasoning against giving access to operator-level damage output, since that opens the door for a cycle of mods continually one-upping each other and removing the utility of the /kill command. With that being the case, I think the conversion itself is unfounded in a weaker state, since combining hyperion w/ the primordial purifier would be roughly equivalent to the weaker case and rewards the player for performing the (tbf fairly simple) combo, instead of just handing them the combo in a single augment.

With the capability stuff, I haven't had problems with it that are relevant to this (believe me, I know your pain when it comes to client syncing), but it is worth noting that the previous system wouldn't be reliable with storing things in the instance anyway, since forge relies on nbt serialize/deserialize methods for persistent data storage. For example, when an itemstack is copied (which happens a lot), forge doesn't just put references to the old stack's capabilities in the new stack; instead, it initializes new capabilities for the new stack, then deserializes the capability NBT from the old stack onto the new stack's capabilities.

And you're right, the vulnerability stack concept is very similar to the frenzy augment. Since part of the plan is to support gauntlet augments on the cannon, I think letting the frenzy augment fill that niche is a better idea, especially if we allow more than one gauntlet augment to be mounted.

I think for now I'll just leave the charge rifle without a sound, and then ask TechnoMysterio to create one once the functionality of the augment is more or less finalized.

@M-W-K
Copy link
Author

M-W-K commented Jan 16, 2025

I was just looking through JEI and had an idea for three related augments, one for the shimmerleaf, cinderpearl, and vishroom flora. Shimmerleaf's augment would cause the cannon to apply weakness and slowness to hit enemies, cinderpearl's would set them on fire, and vishroom's would apply poison.

@TheCodex6824
Copy link
Owner

With the capability stuff, I haven't had problems with it that are relevant to this (believe me, I know your pain when it comes to client syncing), but it is worth noting that the previous system wouldn't be reliable with storing things in the instance anyway, since forge relies on nbt serialize/deserialize methods for persistent data storage. For example, when an itemstack is copied (which happens a lot), forge doesn't just put references to the old stack's capabilities in the new stack; instead, it initializes new capabilities for the new stack, then deserializes the capability NBT from the old stack onto the new stack's capabilities.

Right, capabilities are handled differently. All capabilities that are stateful should already implement INBTSerializable and have their item use a capability provider that is capable of saving NBT, including augments (if it doesn't, that's a bug for the reasons you mentioned). Right now, the impulse cannon augments all use SimpleCapabilityProviderNoSave which as the name suggests, doesn't persist NBT. If you need to store data in the augment instances, switching that over to SimpleCapabilityProvider is fine.

I was just looking through JEI and had an idea for three related augments, one for the shimmerleaf, cinderpearl, and vishroom flora. Shimmerleaf's augment would cause the cannon to apply weakness and slowness to hit enemies, cinderpearl's would set them on fire, and vishroom's would apply poison.

That's a neat idea, and would give more use for those plants. I'm wondering what the lore justification is for the ones other than cinderpearl - I guess shimmerleaf could have something to do with mercury / quicksilver, and maybe vishroom has something to do with the vitium in it?

Also, it seems like you made quite a few changes to the augment API now. This is fine as that was planned to get refactored for the augmentation station anyway, but it's still an API break that has to go in a major release instead of a point release. There's two ways I can think of to work around this for now:

  1. Switch your branch to target the staging branch instead, which is a new one I made that's intended for things that are "done" but have some dependencies on other tasks or otherwise can't be merged directly to master yet. This would require no changes on your end, but the new augments you're making would have to wait until the next feature update with the other API-breaking changes.
  2. Split out the changes that require API changes into a separate PR, and keep the minimally invasive ones as a merge to master. This would be the fastest way to get some augments in the mod, but would require you to do work to split up your PR.

You can choose whatever approach you want (or suggest another one).

@M-W-K M-W-K changed the base branch from master to staging January 23, 2025 17:55
@M-W-K
Copy link
Author

M-W-K commented Jan 23, 2025

I don't know about lore reasons for the silverleaf and vishroom. Maybe I should switch them around? Vishroom applies nausea when you touch it, so it might be a better fit for "miscellaneous negative effects." Silverleaf can be processed into quicksilver which is technically mercury, which is poisonous, so that could fit with poison or even wither. More thought should probably go into those two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants