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

Weapon Selection Modifiers #2804

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

MajorCooke
Copy link
Contributor

Adds the ability for inventory to override weapon selection of player pawns, but also adds a post version of those functions within pawns. This ensures that pawns can have the final say. However, they default to let the inventory modified result through.

RicardoLuis0 and others added 2 commits November 10, 2024 12:16
@madame-rachelle
Copy link
Collaborator

I am dubious about this because the engine is full of overrides for overrides for overrides for overrides - this makes refactoring and code maintenance extremely difficult, I am actually more inclined to say no considering how many overrides for overrides for overrides for other things we already have. At some point you have to say enough is enough and tackle your problems with other methods.

@MajorCooke
Copy link
Contributor Author

MajorCooke commented Nov 10, 2024

There won't be any further overrides of this. But if you have better ideas, I'd love to hear them. Right now, the way it's laid out, I'm not seeing many other alternatives, and I desperately want to stop reading in "slot X" in order to make my mod work. It's a nasty hack that relies upon reading commands executed via keybinds.

@madame-rachelle
Copy link
Collaborator

If this is indeed the last override for at least this section of code and no more will be added after that, then I am suppose I am okay with that, but I don't want another nightmare like damage modifiers, vulnerability/invulnerability, visibility/invisibility, A_Look exit checks, or other things like those.

@MajorCooke
Copy link
Contributor Author

MajorCooke commented Nov 10, 2024

Yep, this is final. However discussions for a potentially better system have come up, want to look into those first.

@MajorCooke MajorCooke marked this pull request as draft November 10, 2024 23:14
@MajorCooke MajorCooke marked this pull request as ready for review November 11, 2024 00:37
@MajorCooke
Copy link
Contributor Author

Nevermind, the idea is unfeasible so I'll be sticking with this.

@Boondorl
Copy link
Contributor

Boondorl commented Nov 11, 2024

We need more context for this. Stuff can be added in if the mod needs it but it should be for things that truly aren't feasible to do from ZScript. I'm dubious that this is needed and it feels like adding a getter for the impending network use item might be vastly more elegant here. Players don't select their weapons instantly but it needs to be networked out first meaning there should be a window where you can capture it and potentially modify it if needed.

I'm also not the biggest fan of this because it puts the ball in every other mod's court to prevent your mod from messing with the player in nearly impossible to predict ways. This is itself a huge problem with my current WIP clientside prediction logic for weapons and modifying the use item would be vastly more feasible for me to manage on the backend than a random chain of virtual functions since I could capture that properly.

@MajorCooke
Copy link
Contributor Author

MajorCooke commented Nov 11, 2024

I'm making a mod where you can override all forms of weapon switching and reorganize weapons into whatever slot players want and in any order.

I'm so tired of all the finagled contraptions that consist of weapon selecting. It's so awful, so I'm making a mod that allows for reorganizing of every weapon no matter what they are, all done in ZForms.

Unfortunately, with the current systems in place, that means the only thing I can do to read slot selections is to get them from InputProcess() to detect whenever slot X, weapnext and weapprev crop up - as that's what I'm doing right now and blocking them from passing while I perform my own weapon switching.

adding a getter for the impending network use item might be vastly more elegant here

If you mean adding a virtual function for these in an event handler, I can agree to that, but if I'm mistaken, please clarify.

@Boondorl
Copy link
Contributor

I don't know how else you would do this except for listening to the weapon slot commands. It sounds like what you want is some way to manually set the ItemUse Actor since you could then set it from UI the same way it does internally, in which case you'd more or less just be ZScriptifying the commands. This current solution feels like overkill, especially for an exceedingly niche scenario

@RicardoLuis0
Copy link
Collaborator

I don't know how else you would do this except for listening to the weapon slot commands. It sounds like what you want is some way to manually set the ItemUse Actor since you could then set it from UI the same way it does internally, in which case you'd more or less just be ZScriptifying the commands. This current solution feels like overkill, especially for an exceedingly niche scenario

this is the cleaner way of doing it, overriding the weapon choice virtuals universally from inventories instead of listening to the weapon slot commands, overriding itemuse wouldn't work because of multiple weapons in the same slot would need to stay in the same slot together, or else the selection would completely break down

@MajorCooke
Copy link
Contributor Author

Hmmm. What about just a PlayerPicking<Next/Prev>Weapon(Weapon current, int slot, bool checkammo) within an event handler then? It would be fewer function calls to say the least.

This would happen before the player virtual, and it should give just enough information that I need to make my mod work. No double overrides needed either.

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.

4 participants