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

Update Event.lua for develop #1346

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

WolfKann
Copy link
Contributor

@WolfKann WolfKann commented Apr 6, 2024

This adds Is_No_Targeted, Is_No_Draw,
and also adds variables to detect Int/Float/Bool/String NetVars, while also adding an advanced version in the rare chance two netvar types share a name.

This adds Is_No_Targeted, Is_No_Draw,
and also adds variables to detect Int/Float/Bool/String NetVars,
while also adding an advanced version in the rare chance two netvar types share a name.
lua/pac3/core/client/parts/event.lua Outdated Show resolved Hide resolved
lua/pac3/core/client/parts/event.lua Outdated Show resolved Hide resolved
lua/pac3/core/client/parts/event.lua Outdated Show resolved Hide resolved
Fixed Code from Pingu7867's Suggestions
@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 6, 2024

I believe I've incorporated the changes successfully, I left the tostring() parts in because personal experience has seen them break. as stupid as it sounds, It has saved headaches as a codebreaker so it's optional if you want to take the extra steps, but advised.

Forgot Userdata prefix
fixed StringOperator (I really need to proof read better, but it's 3:40 AM and I haven't slept yet.)
lua/pac3/core/client/parts/event.lua Outdated Show resolved Hide resolved
Changed // to --
@CapsAdmin
Copy link
Owner

Just a heads up, but this will probably allow cheating in some non-sandbox gamemode.

Maybe we need a way to disable some events and proxies in non-sandbox gamemodes by default like some features already are.

@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 7, 2024

You could make them tied to a server-side command "sv_pac3_allow_netvars"

@pingu7867
Copy link
Collaborator

Just a heads up, but this will probably allow cheating in some non-sandbox gamemode.

Maybe we need a way to disable some events and proxies in non-sandbox gamemodes by default like some features already are.

It would allow cheating how?? As if the networked vars weren't already public information. The vars are visible in the client realm already, that's what they're for. But clients don't have server write access.

It's not our job to enforce other gamemodes' balance and security of information or to prevent people from accessing information available to the client. We're way past that stage; even pre-combat update there were plenty of ways to "cheat", whatever that means.

NW vars are a sloppy way to do secure server transactions, and if people think it is, they're foolish. And that still doesn't force us to do anything. Secure stuff should be server-centered, clients can make requests but the server is the ultimate authority that decides whether actions that produce changes happen. Bouncing sensitive information around in the client realm is THEIR no-no, not ours.

What I'll propose instead is that, if some servers care so much about game balance in their little corner of a fundamentally sandbox game, they can stop using pac3 or make their own fork that removes features. Erasing stuff is baby-level coding. I won't make a whole feature access control system for everything just because.

@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 7, 2024

That too, NW Vars are server side variables, the only way this could enable cheating is if you could grab variables from other players, or if the variables being grabbed are suppose to be hidden, but regardless, it's not like if I'm playing a TTT game and I do GetNWString("Role") it'll just tell me what anyone's is. this would only tell me my own roles, so unless there's a specific easy to miss but big scenario for it to cause "cheating" you'll have to point it out for us.

@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 7, 2024

and, grabbing variables from other players would require the ability to add pacs to other players to put the event on in the first place, but in that regard you could do much worse if you had those permissions

@pingu7867
Copy link
Collaborator

You could make them tied to a server-side command "sv_pac3_allow_netvars"

I will not humor this level of hyperspecific granular control. I'm not opposed to compromises but I need a good reason before treating anything with seriousness.

Server convars should target something broad or something that has tangible implications. reading available information isn't cheating. And if it is, maybe those servers should rethink using pac3, or actually... enforce their rules!

What's the percentage of cheating players anyway? Something only needs to be done if the problem numbers are big enough. Even if we highball it, I still think the principle of proportionality will still be in my favor.

@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 7, 2024

You could make them tied to a server-side command "sv_pac3_allow_netvars"

I will not humor this level of hyperspecific granular control. I'm not opposed to compromises but I need a good reason before treating anything with seriousness.

Server convars should target something broad or something that has tangible implications. reading available information isn't cheating. And if it is, maybe those servers should rethink using pac3, or actually... enforce their rules!

What's the percentage of cheating players anyway? Something only needs to be done if the problem numbers are big enough. Even if we highball it, I still think the principle of proportionality will still be in my favor.

I'm not saying I agree with it, but I merely offered a solution if it's such a big concern.

@thegrb93
Copy link
Collaborator

thegrb93 commented Apr 7, 2024

TTT and darkrp probably has some nwvars they don't want visible.

@CapsAdmin
Copy link
Owner

Just a heads up, but this will probably allow cheating in some non-sandbox gamemode.
Maybe we need a way to disable some events and proxies in non-sandbox gamemodes by default like some features already are.

It would allow cheating how?? As if the networked vars weren't already public information. The vars are visible in the client realm already, that's what they're for. But clients don't have server write access.

It's not our job to enforce other gamemodes' balance and security of information or to prevent people from accessing information available to the client. We're way past that stage; even pre-combat update there were plenty of ways to "cheat", whatever that means.

NW vars are a sloppy way to do secure server transactions, and if people think it is, they're foolish. And that still doesn't force us to do anything. Secure stuff should be server-centered, clients can make requests but the server is the ultimate authority that decides whether actions that produce changes happen. Bouncing sensitive information around in the client realm is THEIR no-no, not ours.

What I'll propose instead is that, if some servers care so much about game balance in their little corner of a fundamentally sandbox game, they can stop using pac3 or make their own fork that removes features. Erasing stuff is baby-level coding. I won't make a whole feature access control system for everything just because.

Networked variables are not assumed to be public information because they cannot be read without the server enabling clientside lua.

This is a bit like saying wall hacks are not cheats because the information is already in the client.

I don't know the consequences of this, I just wanted to give a heads up. Maybe you can fix it later after someone comes up with a solution.

@pingu7867
Copy link
Collaborator

pingu7867 commented Apr 7, 2024

TTT and darkrp probably has some nwvars they don't want visible.

That's still not quantifying or proving anything. Anyone can say the names of some of the most common gamemodes.

I want numbers. Even if they're speculated. Give me your estimated frequency of conscious exploiters in the overall population of these servers. How big of a problem do you think it is? What sort of situation do we have, in your opinion?
-1 person every day, which can be banned under 15 minutes? That's a healthy server, we don't need to do anything if it's small like that.
-1 per hour? worse, but they could still do something about it, such as the minor code edits I mentioned. Learning that this obscure and niche "pac netvars event enable" convar exists is as easy as removing some lines of code (very easy), so I don't see why I should feel a particular strong duty about protecting other gamemodes if they can't be expected to do it.
-something bigger but undetectable and insidious that's hard to stamp out? sucks, but what can I do, other than telling them to rethink their selection of addons especially pac3. if they have that many exploiters lurking around, they got bigger problems already.

I still haven't heard a convincing argument for why I should care about people I don't know, and why that's a valid reason to disable our features by default when the alternative works just as well. It's easy to edit our code. It's open and somewhat documented. And why I grandstand so much is that having super precise settings down to event or proxy availability, I consider this as code bloat. But I guess the argument is that everyone (server owners etc.) is so thoroughly uninformed that none of that matters, and all that matters is that as soon as anyone can think of a theoretical way to abuse a pac feature, it should be disabled by default and put behind a convar.

@pingu7867
Copy link
Collaborator

Networked variables are not assumed to be public information because they cannot be read without the server enabling clientside lua.

This is a bit like saying wall hacks are not cheats because the information is already in the client.

I don't know the consequences of this, I just wanted to give a heads up. Maybe you can fix it later after someone comes up with a solution.

I mean I know the solutions, I know what can be done. Still, I'm adamant about proof and knowing how big of a problem we're talking about. Not in terms of potential, but the actual impact. You're not immune from that. I get it, a "heads up" is just a thing that pops up in your mind and giving a quick warning about stuff, you don't mean anything more by it. But that changes when you propose a deeper rework of pac's code and/or splitting off features which will inevitably multiply the convars, which I rarely think is a good idea. only when there's good reason and no alternative.

I'll figure out something that makes sense. But we're discussing how much and I want to know why it'd be up to us and not servers to figure out their security and adjust when abusers arrive (I think they can react just fine, so it's not a problem if it doesn't happen too often).

@WolfKann
Copy link
Contributor Author

WolfKann commented Apr 7, 2024

Just a heads up, but this will probably allow cheating in some non-sandbox gamemode.
Maybe we need a way to disable some events and proxies in non-sandbox gamemodes by default like some features already are.

It would allow cheating how?? As if the networked vars weren't already public information. The vars are visible in the client realm already, that's what they're for. But clients don't have server write access.
It's not our job to enforce other gamemodes' balance and security of information or to prevent people from accessing information available to the client. We're way past that stage; even pre-combat update there were plenty of ways to "cheat", whatever that means.
NW vars are a sloppy way to do secure server transactions, and if people think it is, they're foolish. And that still doesn't force us to do anything. Secure stuff should be server-centered, clients can make requests but the server is the ultimate authority that decides whether actions that produce changes happen. Bouncing sensitive information around in the client realm is THEIR no-no, not ours.
What I'll propose instead is that, if some servers care so much about game balance in their little corner of a fundamentally sandbox game, they can stop using pac3 or make their own fork that removes features. Erasing stuff is baby-level coding. I won't make a whole feature access control system for everything just because.

Networked variables are not assumed to be public information because they cannot be read without the server enabling clientside lua.

This is a bit like saying wall hacks are not cheats because the information is already in the client.

I don't know the consequences of this, I just wanted to give a heads up. Maybe you can fix it later after someone comes up with a solution.

The problem with this mindset is your comparison,

the network variables that are shared by this addition are only your own. which your own client already knows and uses, when comparing "wall hacks" it's obvious that your position on the situation would be correct, but that's not what these variables are for, nor can these variables allow for that kind of behavior from the get go.. These variables are used for things like your last log in time, how much money you have, what's your bonuses or luck is. it's entirely depending on what game mode you're coding with has it as. Bigger Gamemodes however don't even use these variables for that stuff. a Popular one, Nutscript for example has it's own network system for variables that this doesn't detect.

On top of that, I can say confidently probably the top 4 popular game modes there isn't anything you can pull from these variables that wouldn't be information you already have, as again previously stated, unless you have Pac3 enabled to where you can add these events onto other players, you can't get their variables. and, presuming you COULD do that, with the right permissions, you could easily add a halo, or add an effect, or do another assortment of stuff without even needing to use the variable system to just detect something.

Pingu7867 has a point, from a development standpoint it's simply moot to try and ConVar lock this stuff realistically, since pac3 is open source and it is easy to just comment out lines of code or delete things if you don't want them to be used. and in that exact scenario, why would we risk not adding a feature that could be extremely useful in making pac3 more Compatible with other game modes outside of sandbox, if not also including sandbox.

lastly, the Variable list you receive from this is only the names, that doesn't mean you'll know what they do, what the values are or stand for, this implementation would imply that whoever is using this has been either on server enough to know what they're doing, and by that point, most trolls would have already done something much worse in that timespan. so there really isn't a need for it.

@pingu7867
Copy link
Collaborator

the network variables that are shared by this addition are only your own

Actually, I kind of want read access to other entities' NW vars. But try_viewmodel(ent) doesn't give you the playerowner ("player owner" means that because it's your pac, it's your part so it's owned by you), but root owner ("root owner" means that, despite it being your pac, your part, the group is "owned" by (assigned to) another entity aka prop outfits). You can still apply parts to other players. maybe not wear (pac_sv_prop_outfits is off by default but allows that if you set it all the way to 2) but you can load the parts in the editor.

So despite my previous firebrand attitude, I would settle down on my solution: pac_sv_danger_mode, a bigger convar to disable minor abusable features: NW vars (get information), nearest_life (targets nearby npcs/players, negating stealth. yes, being behind walls is an important part of many sorts of gameplay, I'm not gonna deny that one) and whatever else comes up.

This will be the only convar we'll add for what I might call "small controversial features". Instead of having these discussions repeatedly, I'd rather get that done right then and there. I want a list, do it and then hear not a peep. Let's accept the fact that RP server owners etc. want control over pac3 for their server. It's true. Then if they want to limit one thing, they will also limit other stuff. In no world will it ever happen that they'd allow one but not another. It's just splitting hairs at this point. So a broad convar is the appropriate solution.

After that, it's just making sure to inform people about what happens (I made an editor popup system in prevision of stuff like this. So when they load an event that is known to be a "spicy event" it could tell people that they're in baby server mode and some of their stuff wouldn't work, and users can then pester staff about it, it's not our affairs), and making sure it doesn't break people's data. There's problems with how pac handles data across versions and loading/saving parts. Unregistered events lose some information (the event's type) on load, and parts don't save missing information (the famous "base" part class some combat beta testers have had the displeasure of knowing when switching servers). Why is this a major issue? autosave. For people relying on autosave but being unaware of the one-way nature of saving operations, one moment you're working on your pac, and the next you silently lose your work because some parts aren't saving properly. But you don't notice it until it's too late, because your pac is big and has nice categories hiding stuff away but you've been working a lot on that pac so the autosave list is exhausted and cleaned up. We really have to be careful about these things.

why would we risk not adding a feature

We're not "risking" anything, it'll just be some missed opportunities, nothing is lost there. But what you don't get is that apparently most people in other gamemodes don't actually care about pac3 as an artform of technical feats. They have a socially-centered gameplay goal to fulfill and they'll use what they see, no more. If a feature is not yet given to them and visible to them, they won't notice a thing (unless they're singleplayer-focused), and won't care either way.

They only start caring if you give them a feature that's easily visible and highly-desired but take it away later. It's not like that time I added take_damage and inflicting_damage events in my beta combat fork, but then got scolded and removed it in the final PR back to mainline develop. People (those who care enough to beta test the combat update, not RP/TTT etc. servergoers) loved these but it required too much networking which was deemed not worth it. The impact was actually felt when they saw their thing taken away.

It's not like people noticed some of the other things I did either. Not a single soul seems aware of the extensive editor customization features I added. Features are not all equal. The combat update beta was reasonably widespread to those that mattered, I see now the impact: That's what a window of time with available features does when it's taken away afterwards. Even if it's short-ish, that window of time is enough for people to cling to it. Why the tangent? Attention and psychology. They're strong factors. Not sure where I was going with this but there's multiple things to consider.

@pingu7867
Copy link
Collaborator

I will take the liberty of applying some changes.

also fix main get_networked enum builder to provide key-key pairs instead of key-values. the name list should give keys. the value is tested in result
for minor features that can be construed as having an impact on gameplay

when implementing these controls, expand the list as needed instead of making ultraspecific convars
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