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

EquipmentEditor: Set clip on buy #1499

Merged
merged 13 commits into from May 11, 2024
Merged

EquipmentEditor: Set clip on buy #1499

merged 13 commits into from May 11, 2024

Conversation

TimGoll
Copy link
Member

@TimGoll TimGoll commented Mar 31, 2024

Added SWEP.SetClipOnBuy and SWEP.ClipOnBuy to set the weapon's clip on buy via the equipment editor. I did not want to just use the latter variable to set a clip on buy to all weapons as most "one time use weapons" instantly remove the weapon on use which makes this slider pointless. It is therefore something swep makers have to integrate into their weapon, which is pretty simple.

I also reordered some elements in the equipment menu because they were out of place. It now looks like this:

image

@TimGoll TimGoll added the type/enhancement Enhancement or simple change to existing functionality label Mar 31, 2024
@TimGoll TimGoll added this to the v0.14.0b milestone Mar 31, 2024
lua/terrortown/lang/en.lua Outdated Show resolved Hide resolved
TimGoll and others added 2 commits April 7, 2024 14:31
Co-authored-by: Histalek <16392835+Histalek@users.noreply.github.com>
@saibotk
Copy link
Member

saibotk commented Apr 8, 2024

Im opposed to the networking part, can you quickly explain why that is necessary?

@TimGoll
Copy link
Member Author

TimGoll commented Apr 9, 2024

Im opposed to the networking part, can you quickly explain why that is necessary?

It is needed to update the clip variable on the client as well. This was the easiest solution I could find at least. As @ZenBre4ker said the saved keys are networked, but I have to set wep.Primary.ClipSize to the value of wep.ClipOnBuy if wep.SetClipOnBuy is set to true. I could probably also achieve this with the already networked data and a hook/timer combo. But I did not investigate this. I'm also not at home for the coming two weeks. So if someone wants to look into it, please go ahead, I'm only available via text though

@TimGoll
Copy link
Member Author

TimGoll commented Apr 20, 2024

@saibotk Just to ping you as you did not answer to my comment

@saibotk
Copy link
Member

saibotk commented Apr 20, 2024

But why does the client need this? What happens if you dont do that?

Seems like something that gmod should/would handle.

@TimGoll
Copy link
Member Author

TimGoll commented Apr 20, 2024

But why does the client need this? What happens if you dont do that?

Seems like something that gmod should/would handle.

The Playerinfo HUD element is bugged and shows the wrong max clip size. GMod does not handle this, GMod sadly has no max clip. This is manually handled by TTT

@saibotk
Copy link
Member

saibotk commented Apr 20, 2024

Can this be solved via the weapon entity's network variables? Like we do with the sprint system?
Seems like the more correct place for this instead of another side channel network update.
If not im fine with this. Thanks for explaining!

@TimGoll
Copy link
Member Author

TimGoll commented Apr 20, 2024

Can this be solved via the weapon entity's network variables? Like we do with the sprint system? Seems like the more correct place for this instead of another side channel network update. If not im fine with this. Thanks for explaining!

Not sure, hm. It is not something that is updated on the fly though. Maybe I can use the synced data from @ZenBre4ker's settings stuff, but I did not investigate this and he did not yet answer

@ZenBre4ker
Copy link
Contributor

ZenBre4ker commented Apr 24, 2024

So, Im wondering why this is something TTT2 should implement?
Isnt that a default setting when you get equipment, that it can have ammo directly?

Otherwise while cliponBuy is synchronized it is just a static setting.
But generally you would only need to check it on the client on the receiving end and add that amount to your clip.
Not really necessary to send it to the client additionally.

@TimGoll
Copy link
Member Author

TimGoll commented Apr 24, 2024

So, Im wondering why this is something TTT2 should implement?
Isnt that a default setting when you get equipment, that it can have ammo directly?

Yes. But there are multiple weapons that want to have a configurable clip size. Using the .Primary.Clip variable doesn't work because it is only updated on weapon initialization when the file is loaded (so only once per map change) and it would also break weapons that do not account for dynamic clip size.

I therefore added the ClipOnBuy variable that is only used if the weapon creator enables it. It can be used so that the defi has a definable amount of uses without the hacking that is currently in place.

Not really necessary to send it to the client additionally.

As I said in a previous comment that does not work. If you do not set the MaxClip on the client manually, it ends up showing stuff like "4 / 1" (four out of one) left, which I consider a bug.

I'm not sure if I actually answered your question though as I already wrote the same in the description and the previous comments. So you might have to clarify your question a bit more

Edit: I think I got your second part. I'm not sure how to use it though. Is there a hook or a callback when the data arrived on the client? I don't want to poll it in a think hook for example

@ZenBre4ker
Copy link
Contributor

There should be something in the database you can hook to.

@TimGoll
Copy link
Member Author

TimGoll commented Apr 24, 2024

There should be something in the database you can hook to.

All I could find is this: https://github.com/TTT-2/TTT2/blob/master/lua/ttt2/libraries/database.lua#L291

But as far as I can see this is only called in the realm where it is stored (the server in this case) and it is also not called when the weapon is initialized and the value from the database is only read, not changed

@ZenBre4ker
Copy link
Contributor

ZenBre4ker commented Apr 24, 2024

Mhh, normally it should be synced. Same way we do it for "database"-element in the ui.

@TimGoll
Copy link
Member Author

TimGoll commented Apr 24, 2024

Mhh, normally it should be synced. Same way we do it for "database"-element in the ui.

I've never worked with your synced database stuff. So if you think there is a better solution to my custom syncing, I'd really appreciate pointers that show me what to do because I'm not that motivated to start working through all the database and syncing stuff you did, that was quite a lot. Thanks!

@ZenBre4ker
Copy link
Contributor

I Thought about this here, should work the same for your case:

database.AddChangeCallback(
ShopEditor.accessName,
name,
nil,
function(accessName, itemName, key, oldValue, newValue)
if not istable(equipmentTable) then
database.RemoveChangeCallback(ShopEditor.accessName, name, nil, callbackIdentifier)
return
end
equipmentTable[key] = newValue
end,
callbackIdentifier
)

@TimGoll
Copy link
Member Author

TimGoll commented Apr 24, 2024

Thank you, I will check it out!

@TimGoll
Copy link
Member Author

TimGoll commented May 8, 2024

I simplified the syncing a lot by using the syncing that sven already implemented

@TimGoll TimGoll requested review from saibotk and ZenBre4ker and removed request for saibotk and ZenBre4ker May 8, 2024 18:38
Co-authored-by: ZenBre4ker <72046466+ZenBre4ker@users.noreply.github.com>
@TimGoll
Copy link
Member Author

TimGoll commented May 9, 2024

Before merging I will probably rename the variable. I also have to test if the UI is working as intended, I think I forgot to set the MaxClip. But that is an easy fix.

Just so you are aware and don't merge it yet.

@TimGoll
Copy link
Member Author

TimGoll commented May 9, 2024

Tested and works. @ZenBre4ker feel free to merge once your comments are resolved.

image

@TimGoll TimGoll merged commit 45872ff into master May 11, 2024
4 checks passed
@TimGoll TimGoll deleted the equip-on-buy branch May 11, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Enhancement or simple change to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants