TTT2 icon indicating copy to clipboard operation
TTT2 copied to clipboard

EquipmentMenu: Added check before function call

Open TimGoll opened this issue 1 year ago • 11 comments

Last night I discussed some things with @mexikoedi. One thing we noticed that there are quite a few badly done weapons for TTT that forget to set the weapon base correctly. While this is bad practice and shouldn't be done, most of these weapons still work just fine. Adding this check here makes sure that this populate function is only called if it actually uses the correct base.

TimGoll avatar Feb 12 '24 07:02 TimGoll

how can something set their base as something other than weapon_tttbase and still be set accordingly? don't we explicitly filter out things that aren't based on weapon_tttbase? shouldn't we check or correct this even higher up the chain rather than mitigate consequences so they're even harder to detect down the chain?

EntranceJew avatar Feb 12 '24 08:02 EntranceJew

how can something set their base as something other than weapon_tttbase and still be set accordingly? don't we explicitly filter out things that aren't based on weapon_tttbase? shouldn't we check or correct this even higher up the chain rather than mitigate consequences so they're even harder to detect down the chain?

This addon for example uses their own base: https://steamcommunity.com/sharedfiles/filedetails/?id=2705642928

There are quite a few of them and realistically no one will update them. We could also throw them out of the valid weapon list, but I think that will only result in complaints by players.

My decision here to do it that way is founded on the fact that this weapon is working perfectly fine, it is just causing an error in the F1 menu:

[ttt2-test-0.13] addons/ttt2-test-0.13/lua/terrortown/menus/gamemode/equipment/base_equipment.lua:189: attempt to call method 'AddToSettingsMenu' (a nil value)
  1. Populate - addons/ttt2-test-0.13/lua/terrortown/menus/gamemode/equipment/base_equipment.lua:189
   2. BuildContentArea - addons/ttt2-test-0.13/gamemodes/terrortown/gamemode/client/cl_help.lua:319
    3. GenerateSubmenuList - addons/ttt2-test-0.13/gamemodes/terrortown/gamemode/client/cl_vskin/vgui/dsubmenulist_ttt2.lua:149
     4. OnValueChange - addons/ttt2-test-0.13/gamemodes/terrortown/gamemode/client/cl_vskin/vgui/dsubmenulist_ttt2.lua:72
      5. OnValueChange - addons/ttt2-test-0.13/gamemodes/terrortown/gamemode/client/cl_vskin/vgui/dsearchbar_ttt2.lua:42
       6. unknown - lua/vgui/dtextentry.lua:194

Also, I want to add here, that this error is pretty unhelpful to the average developer because it doesn't point in any way to their weapon. You have to understand the depths of the TTT2 code base in order to know the reason behind this error.

TimGoll avatar Feb 12 '24 08:02 TimGoll

However, we should probably iterate over all weapons and check their base. If it isn't the correct one, then we should throw an error in the addon checker. I think that will get best of both worlds

TimGoll avatar Feb 12 '24 08:02 TimGoll

This addon for example uses their own base: https://steamcommunity.com/sharedfiles/filedetails/?id=2705642928

But this one doesn't even reference weapon_tttbase because it immediately overwrites it, my understanding was these lines were true and we were meant to eject things that did not even utilize the weapon_tttbase:

Also, I want to add here, that this error is pretty unhelpful to the average developer because it doesn't point in any way to their weapon. You have to understand the depths of the TTT2 code base in order to know the reason behind this error.

i agree with this, i just thought we were more selective with what does and doesn't get rejected

EntranceJew avatar Feb 12 '24 08:02 EntranceJew

This addon for example uses their own base: https://steamcommunity.com/sharedfiles/filedetails/?id=2705642928

But this one doesn't even reference weapon_tttbase because it immediately overwrites it, my understanding was these lines were true and we were meant to eject things that did not even utilize the weapon_tttbase:

Also, I want to add here, that this error is pretty unhelpful to the average developer because it doesn't point in any way to their weapon. You have to understand the depths of the TTT2 code base in order to know the reason behind this error.

i agree with this, i just thought we were more selective with what does and doesn't get rejected

huh, then this part of the code is not working. I did not know our policy was to reject all weapons that are not based on the weapon_tttbase. The we should probably change that

HOWEVER this weapon in particular isn't buyable, it is spawned in the world. Maybe there is the issue

Edit: I further studied the code and to me it seems the skip weapon change is only applied to primary fire. Basically it modifies the next time a weapon can shoot and that seems to be it.

TimGoll avatar Feb 12 '24 08:02 TimGoll

This addon for example uses their own base: https://steamcommunity.com/sharedfiles/filedetails/?id=2705642928

But this one doesn't even reference weapon_tttbase because it immediately overwrites it, my understanding was these lines were true and we were meant to eject things that did not even utilize the weapon_tttbase:

Also, I want to add here, that this error is pretty unhelpful to the average developer because it doesn't point in any way to their weapon. You have to understand the depths of the TTT2 code base in order to know the reason behind this error.

i agree with this, i just thought we were more selective with what does and doesn't get rejected

huh, then this part of the code is not working. I did not know our policy was to reject all weapons that are not based on the weapon_tttbase. The we should probably change that

HOWEVER this weapon in particular isn't buyable, it is spawned in the world. Maybe there is the issue

It spawns in the world and is also buyable by default. But yes the error is only a bit annoying for example if you want to edit something in the "Equipment menu" and use the search bar or click on the addon then the error pops up but otherwise this "weapon" seems to be working fine.

mexikoedi avatar Feb 12 '24 08:02 mexikoedi

Should definitely still consider removing things that share no lineage with weapon_tttbase, it'll be even more difficult to keep things sorted -- if a user is playing peer to peer with friends all their sandbox addons will leech into TTT and trip errors, because very few people set their weapons/ents to go into /gamemodes/terrortown/, it can get really messy.

In this circumstance, most of the MHS base is just SCK code that could be ported to the new code @TimGoll just introduced to the weapon_tttbase and isn't providing any critical functionality.

EntranceJew avatar Feb 12 '24 09:02 EntranceJew

Should definitely still consider removing things that share no lineage with weapon_tttbase, it'll be even more difficult to keep things sorted -- if a user is playing peer to peer with friends all their sandbox addons will leech into TTT and trip errors, because very few people set their weapons/ents to go into /gamemodes/terrortown/, it can get really messy.

In this circumstance, most of the MHS base is just SCK code that could be ported to the new code @TimGoll just introduced to the weapon_tttbase and isn't providing any critical functionality.

I completely agree with you here. But we also have to be realistic, no one is going to update these addons. Maybe mexikoedi will, but not the original dev

TimGoll avatar Feb 12 '24 09:02 TimGoll

I completely agree with you here. But we also have to be realistic, no one is going to update these addons. Maybe mexikoedi will, but not the original dev

As a separate effort from the addonchecker, if there are old addons that people would like to see brought forward that have no equivalent then we should make a form or something to just be made aware of addons which aren't up to par with TTT2 features even if they're not "breaking" things. Things which could be using the MSTACK / STATUS libs or the new enhanced bases, or anything that is using SCK code that could be gutted and given tooltips / more consistent controls.

EntranceJew avatar Feb 12 '24 09:02 EntranceJew

I completely agree with you here. But we also have to be realistic, no one is going to update these addons. Maybe mexikoedi will, but not the original dev

As a separate effort from the addonchecker, if there are old addons that people would like to see brought forward that have no equivalent then we should make a form or something to just be made aware of addons which aren't up to par with TTT2 features even if they're not "breaking" things. Things which could be using the MSTACK / STATUS libs or the new enhanced bases, or anything that is using SCK code that could be gutted and given tooltips / more consistent controls.

I'm 100% with you that the addonchecker should warn if the weapon uses the wrong base. Besides that I'm open to removing them completely like you suggested or doing it as mexikoedi suggested. I think @ZenBre4ker @saibotk and @Histalek should voice their opinion on this as well as this is a pretty important decision imho.

TimGoll avatar Feb 12 '24 09:02 TimGoll

@Histalek please confirm that my dev message is to your liking

TimGoll avatar Feb 14 '24 20:02 TimGoll