TheCodex6824
Expanded Impulse Cannon Augmentation
Draft PR for code that implements and tweaks content described in #393 Associated tasks:
- [x] Get the impulse cannon (augment) related API to a state compatible with most of the changes needed.
- [x] Implement augments that were more or less okayed during the discussion in #393 and their research
- [x] Implement conversions that were okayed during the discussion in #393 and their research
- [x] 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.
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).
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.)
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.
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.
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.
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:
- Switch your branch to target the
stagingbranch 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. - 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).
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.
@TheCodex6824 I've decided in an effort to avoid this PR sitting here until the end of time to just clean up what I've already done and get it in a mergeable state, and add any further augments in the future. Could you do a once-over everything, then I'll revert the commit with the RFG stuff on it so it can run on your device?
Once I've reverted the commit i'll be unable to run the branch in dev unless I revert the revert, which would be awkward, so I'd like to make sure everything is good to go before then. After RFG is gone, I'll also fix things that break in the aftermath that prevent the project from building (e.g. I believe FastUtils, which I have made use of, is added as a dependency by RFG. Do you want to add this as a dependency of the project or stop using it entirely?)
The new conversions also are still using placeholder sounds / not using sounds at all, but I'm not really capable of making new ones on my end. Do you have recommendations as to what I should do for these?
That works for me. I'm currently looking it over, I'll let you know when it's all good.
As for the cleanup part - I believe FastUtils is a normal Forge dependency, although I'm not sure if RFG is pulling in a newer version. If everything compiles with that after removing RFG, I'd say it's fine. When the time comes, feel free to take advantage of the github actions runner and push changes to the PR to test them, or if all else fails I can take a look.
For the sounds, the placeholders you have are fine. I will replace them before merging this back to the main branch.
Everything looks good code-wise. I compiled a test version to see how it works ingame and most things look good there as well (and I still wish I made the railgun work like the recursion conversion...). The only issue I found was that the gyroscope seems to sometimes not aim correctly when multiple entities are involved. I can have my crosshair over the hitbox of an entity, and it will veer away and aim at something else instead. Here's a screenshot of the most egregious example I could get pictures of:
The yellow line was drawn by me for convenience to see where the crosshair was before recoil. It was over the Creeper closer to the foreground, but then went a bit towards the Creeper further away, but didn't hit any of them.
There was also one instance where I was aiming at a close Creeper like in that screenshot, but then it went slightly to the right and hit a spider far away in the background instead. I didn't get a picture of that, but it did do a good job of hitting the spider at least.
@TheCodex6824 It looks like my code for selecting entities that the gyroscopic adjuster can move aim to was absolutely atrocious, so I replaced it with something that ought to do what I intended it to. After doing so I couldn't see any examples of the adjuster picking an obviously wrong target, can you confirm on your end?
Also, the algorithm doesn't weight distance to target, purely the angle of correction. Theoretically, even if you're aiming at something close up, if something really far away would be less divergence from the crosshair, it will pick that really far away target. Is this desirable or do you want distance to also be considered?
And note number three, I do like the railgun conversion as an alternative to the recurse conversion. The up-front damage potential of the railgun is really useful, compared to the need to charge recurse.
I put together a debug display system to confirm that everything is working as correctly for the gyroscopic adjuster. Since it does appear to be working properly, I've gone ahead and reverted the RFG commit, so this PR should be ready for review and merge.