Added the ability to add callbacks to ConVar that will be called when ConVar is changed. #421
Conversation
|
Very neat addition! |
|
Source.Python/src/core/modules/cvars/cvars.h Lines 97 to 102 in 39ca701 |
|
Works great! Though you forgot a callable check when registering callbacks. The following should raise: ConVarChanged(ConVar("test", "1", "test convar."))(None)Because I haven't investigated as to why, but the following crashes on reload: from commands.server import ServerCommand
from cvars import ConVar
from cvars import ConVarChanged
@ServerCommand('test_command')
def test_command(*args):
...
@ConVarChanged(ConVar('test_command'))
def on_test_command(convar, old_value, new_value):
...Likely because the associated Also, would not this be better to extend @OnConVarChanged
def on_test_command(convar, old_value, new_value):
"""Called when any convar changes."""
@OnConVarChanged('test')
def on_test_command(convar, old_value, new_value):
"""Called when test changes."""I think this could be best, as it currently feel like reinventing the wheel to me. |
Added callable confirmation.
If we can compromise the backward compatibility of OnConVarChanged, I will do so. I also thought about it but gave up because OnConVarChanged doesn't have |
This reverts commit 94e24ba.
Prevented the creation of invalid ConVars. 080d8ca |
You cannot override a command with a convar due to how the internal linking of the engine works but the other way around should be possible. I think the problem is actually there: Source.Python/src/core/modules/commands/commands_server.cpp Lines 162 to 166 in 39ca701 It should be: // Get the ConCommand instance
ConCommand* pConCommand = g_pCVar->FindCommand(m_Name);
if (pConCommand) {
// Unregister the ConCommand
g_pCVar->UnregisterConCommand(pConCommand);
}Because EDIT: Fixed into 1604995.
I would rather discard |
No it won't work, ConVar and ConCommand are incompatible. We need to add a check to ConCommand just as I added a check at Code: from commands.server import ServerCommand
from cvars import ConVar
from cvars import cvar
test_command = ConVar("test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))Output:
And I don't think this fix(1604995) is necessary if the ConCommand is valid.
The current implementation is broken, but that's exactly why we need Example: from cvars import ConVarChanged
@ConVarChanged("bot_quota")
def on_bot_quota(convar, old_value):
convar.set_int(1)test2/test2.py from cvars import ConVarChanged
@ConVarChanged("bot_quota")
def on_bot_quota(convar, old_value):
new_value = convar.get_int()
if new_value < 1 or new_value > 5:
print("bot_quota maximum value is 5.")
convar.set_int(5)Output1 Output2 It is not good to have the behavior change depending on the loading order of the plugins, and such a warning should be useful when debugging. Also, if ConVarChanged is available, OnConVarChanged should only be used in very limited situations. Wouldn't it be better to deprecate OnConVarChanged and move to ConVarChanged? |
You are trying to override a from commands.server import ServerCommand
from cvars import cvar
from cvars import ConVar
from engines.server import execute_server_command
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
assert cvar.find_var('test_command') is None
assert cvar.find_command('test_command') is not None
print('test_command is a command as expected...')
execute_server_command('test_command')
test_command = ConVar('test_command', 'this is the value of test_command')
assert cvar.find_command('test_command') is None
assert cvar.find_var('test_command') is not None
print('test_command is now a convar as expected...')
try:
execute_server_command('test_command')
except ValueError:
print('test_command is not a command anymore and cannot be executed...')
print(f'test_command is set to: {str(test_command)}')080d8ca prevents such behaviour because
It is, because even if we don't override it, another plugin can do so. We only want to unregister valid commands that were created by us and are still valid. |
It doesn't work on my end. Output: Edit: from commands.server import ServerCommand
from cvars import cvar
from cvars import ConVar
from engines.server import execute_server_command
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
execute_server_command('test_command')
test_command = ConVar('test_command', 'this is the value of test_command')
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
try:
execute_server_command('test_command')
except ValueError:
print('test_command is not a command anymore and cannot be executed...')Output: |
What game? It's possible different engine version behave differently. I'm testing on CS:S. |
CS:GO |
|
Since sourcemod was designed to avoid overlapping ConVar, ConCommand, I assumed it would be incompatible with any game. ConVar: ConCommand(ServerCommand): |
|
So, when you do the following: cmd = ServerCommand('foo')(lambda *args: None)
var = ConVar('foo')The behaviours are essentially the same on all games; both the command and the variable get registered. However, the resolution is different. CS:S uses a linked list that resolves from rtl (newer first) while CS:GO (as well as Blade and L4D2) uses a hash list that resolves from ltr (older first). Basically, the following: base = cvar.find_base('foo')Returns var = ConVar('foo')
cmd = ServerCommand('foo')(lambda *args: None)Because we register everything that is queried no question asked: Source.Python/src/core/modules/commands/commands_server.cpp Lines 57 to 61 in ff7819a I will look more into that likely around the week end. |
|
At the end I had time to look more into it and came up with the following that addresses the registration conflicts as well as some other fixes: 02e3490 Here is the code I used to test on CS:S and CS:GO: from commands.server import ServerCommand
from cvars import ConVar
from cvars import cvar
from engines.server import execute_server_command
from memory import get_object_pointer
from gc import collect
collect()
sv_cheats = ConVar('sv_cheats')
sv_cheats.set_bool(False)
@ServerCommand('sv_cheats')
def callback(command):
print('sv_cheats cannot be assigned from the console!')
return False
execute_server_command('sv_cheats', '1')
assert not bool(sv_cheats)
assert get_object_pointer(ConVar('sv_cheats')) == get_object_pointer(sv_cheats)
assert not cvar.find_var('sv_cheats').is_command()
cmd = cvar.find_command('sv_cheats')
assert cmd.is_command()
cvar.unregister_base(cmd)
assert cvar.find_base('sv_cheats') is None
cvar.register_base(sv_cheats)
assert cvar.find_var('sv_cheats') is not None
assert cvar.find_command('sv_cheats') is None
assert get_object_pointer(cvar.find_base('sv_cheats')) == get_object_pointer(sv_cheats)
try:
execute_server_command('sv_cheats', '1')
raise AssertionError
except ValueError:
pass
assert cvar.find_var('foo') is None
foo = ConVar('foo', 'foo')
assert cvar.find_var('foo') is not None
@ServerCommand('foo')
def callback(command):
if command.arg_string == 'baz':
return
execute_server_command('foo', 'baz')
return False
assert str(foo) == 'foo'
execute_server_command('foo', 'bar')
assert str(foo) == 'baz'
del foo
collect()
assert cvar.find_var('foo') is None
foo = ConVar('foo')
assert not foo.is_registered()
print('All assertions passed!') |
I agree that most uses are currently testing for the convar they are looking for, however, I'm strongly against deprecating features or breaking backward compatibility unless absolutely necessary. Having separate decorators also seems redundant to me, especially when extending the current classes to offer the proposed features is not that difficult. The behaviours of the decorator can be achieved with something like that: from cvars import ConVar
class OnConVarChanged:
def __init__(self, callback, *convars):
if isinstance(callback, (str, ConVar)):
convars = (callback,) + convars
callback = None
elif not callable(callback):
raise ValueError('The given callback is not callable.')
self.convars = convars
self.callback = callback
if not convars:
print(f'{callback.__name__} will be called for any convar.')
def __call__(self, callback):
assert self.convars, 'No convars defined on initialization.'
self.callback = callback
print(f'{callback.__name__} will be called for these convars: {self.convars}')
return self
@OnConVarChanged
def on_any_convar_changed():
...
@OnConVarChanged('test', 'test2', ConVar('test3'))
def on_specified_convars_changed():
...
We could really just override #include "code.h"
int get_argcount(object function)
{
object code;
try {
code = getattr(function, "__func__", function).attr("__code__");
} catch (error_already_set &) {
if (!PyErr_ExceptionMatches(PyExc_AttributeError))
throw_error_already_set();
PyErr_Clear();
return -1;
}
if (extract<int>(code.attr("co_flags")) & CO_VARARGS) {
return BOOST_PYTHON_MAX_ARITY;
}
int nArgCount = extract<int>(code.attr("co_argcount")) + extract<int>(code.attr("co_kwonlyargcount"));
return PyObject_HasAttrString(function.ptr(), "__self__") ? nArgCount - 1 : nArgCount;
}print('"lambda a, b, *, c=3: None" =>', get_argcount(lambda a, b, *, c=3: None))
def function(a, b): ...
print('"def function(a, b): ..." =>', get_argcount(function))
def function(*args): ...
print('"def function(a, b, c, *args): ..." =>', get_argcount(function))
class Foo:
def method(self, a, b, c=None): ...
@staticmethod
def staticmethod(a, b, c): ...
print('"Foo.method(self, a, b, c=None): ..." =>', get_argcount(Foo.method))
print('"Foo.staticmethod(a, b, c): ..." =>', get_argcount(Foo.staticmethod))
print('"Foo().method(self, a, b, c=None): ..." =>', get_argcount(Foo().method))
print('"Foo().staticmethod(a, b, c): ..." =>', get_argcount(Foo().staticmethod))
print('"None" => ', get_argcount(None))That info could be cached in your maps so it doesn't have to be retrieved every calls but yeah, I really don't see the necessity of having a separated decorator, deprecating the existing one, or breaking compatibility when it can be worked around rather easily to accommodate both. |
I understand your intentions, but I have a question, why? And, there are multiple problems with this implementation, foremost this does not resolve the conflicts themselves. Code: from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')
assert cvar.find_var('test_command') is not Nonetest2/test2.py from commands.server import ServerCommand
from cvars import cvar
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
assert cvar.find_command('test_command') is not NoneOutput 1: Output 2: If consistency is compromised by the loading order of plugins, then ConVat and ConCommand conflicts should not be allowed from the start. And even if we want to detect changes from the console, we should add a dedicated feature to ConVar, and not allow conflicts with ConCommand. Second, why did you make it possible to delete ConVar? This makes it very easy to cause a crash. Code: from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')
test_command2 = cvar.find_var('test_command')
assert test_command2 is not None
del test_command
test_command2.set_string('new value of test_command')Output: There is no guarantee that the ConVar is retained by Source.Python, so it should not be able to be deleted. |
Your results are expected behaviours. When you load Then, when you load Whatever you pass to that proxy gets assigned to the proxied When you unload Then when you unload Now when you load them the other way around, you get an assertion error because the
Is unregistered because If we were to raise instead of proxying existing or returning unregisered
Because custom |
I am not asking how it behaves, I am asking why you are trying to introduce this behavior. This is no different than you advocating plugin authors to write this kind of code when creating every ConVar. from cvars import ConVar
from cvars import cvar
base_command = cvar.find_base('test_command')
if base_command is not None and base_command.is_command():
cvar.unregister_base(base_command)
test_command = ConVar('test_command', 'this is the value of test_command')get_server_command/ServerCommand and ConVar should always conflict so that one of them is prevented from being created. And, the user should be informed of the conflict as soon as possible, and if the plugin author/user wants to resolve the conflict, they can use cvar.unregister_base to do so.
ConVar must not be deleted under any circumstances. It should always leak. ConVar is not intended to be removed by any implementation. This includes Source.Python. This is a compatibility break. Even if it is not registered, ConVar must be alive until the end of srcds. |
We are responsible to release the memory we use, just like they are for what they allocate. Nobody will do it for us, and they are also freeing theirs as can be shown with a simple hook: from memory import *
from memory.hooks import *
from cvars import *
from commands import *
@PreHook(get_virtual_function(cvar, 'UnregisterConCommand'))
def pre_unregister_concommand(stack_data):
base = make_object(ConCommandBase, stack_data[1])
print(f'>> g_pCvar->UnregisterConCommand({"ConCommand" if base.is_command() else "ConVar"}("{base.name}"))') |
Alright, I thought more about it and I still can't find any valid reason why we should raise. To illustrate my thought process, let's suppose you have the following plugins: # ../test1/test1.py
from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')# ../test2/test2.py
from commands.server import ServerCommand
from cvars import cvar
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
I personally don't see the reason for preventing one or the other to load entirely because this isn't a critical issue whatsoever. Worst case scenario of the former is that
No, definitely not. And such implementation would be highly discouraged. Plugin developers should not care whether their Other than that, yeah, I really cannot justify to prevent one or the other to load in such case. I tried to find scenarios where it would make sense, but I was unable to. Though, perhaps I'm entirely mistaken. I would definitely like for others to chime in and give their opinion on this. |
No, we can't be responsible, and no one can be responsible except srcds.
First of all, UnregisterConCommand does not delete/free a ConVar/ConCommand. I will repeat myself.
Just because the Source.Python plugin created ConVar, it doesn't mean anything. Intentional or unintentional, either way, ConVar can overlap. And since SourcePython's ConVar(str) and SourceMod's CreateConVar returns an existing ConVar, it is not possible to delete ConVar. |
I think you are not considering the case where the conflict between ConVar and ConCommand is unintentional, i.e. by accident. If conflicts occur due to this accident, it is difficult to reproduce and debug the situation. This is because there may be multiple authors implementing each plugin for ConVar and ConCommand.
That's because you have a good understanding of the ConVar and ConCommand specifications, but there's no guarantee that the plugin authors understand them, right? Once a ConVar has been created, it should be retrievable by cvar.find_var, since other plugins may refer to the conflicted ConVar. For example, suppose you have a plugin that accidentally has a conflict, and you have a wrapper plugin for ConVar: Plugin 1: test_command = ConVar('test_command', 'this is the value of test_command')Plugin 2: @ServerCommand('test_command')
def test_command(*args):
print("ConCommand, which has been overridden by accident.")Plugin 3 (Wrapper Plugin): test_command = cvar.find_var('test_command')
assert test_command is not NoneThis will fail. Edited: Plugin test1: sm = ConVar('sm', 'A ConVar that is not intended to be overridden by a ConCommand.')Plugin test2: @ServerCommand('sm')
def sm_command(*args):
print("ConCommand sm")Output: This is an unintended result because the plugin can be loaded when it should fail to load. I used sm for the example, but any ConCommand created by outside of SourcePython will have this problem. There is no end to such edge cases that are not kind to plugin authors. And to avoid such problems, conflicts between ConVar and ConCommand should be prohibited in principle, as in SourceMod. And if the plugin author wants to intentionally introduce conflicts, i.e. override ConVar with ServerCommand, that is up to the plugin author. Finally, and I'll ask this again, why are you trying to introduce this behavior? It seems like an unnecessary and confusing behavior to me. |
They are being unregistered, because they are being released. For example: # ../testing/testing.py
from commands.server import ServerCommand
from cvars import ConVar
sm_flood_time = ConVar('sm_flood_time')
@ServerCommand('testing')
def testing(command):
sm_flood_time.set_float(1.0)Then run @PreHook(get_virtual_function(cvar, 'UnregisterConCommand'))
def pre_unregister_concommand(stack_data):
base = make_object(ConCommandBase, stack_data[1])
print(f'>> g_pCvar->UnregisterConCommand({"ConCommand" if base.is_command() else "ConVar"}("{base.name}"))')
sm_flood_time = ConVar('sm_flood_time')
@PreHook(get_object_pointer(sm_flood_time).make_virtual_function(0, Convention.THISCALL, (DataType.POINTER,), DataType.VOID))
def pre_convar_destructor(stack_data):
print(f'>> ConVar::~ConVar("{make_object(ConVar, stack_data[0]).name}")')
return FalseYou will crash on return due to lack of return type which is not supported by
It actually means everything. It means SP owns its memory, and is responsible to release it. Period. The fact it may or may not be used externally is simply irrelevant and not our responsibility whatsoever. What is our responsibility, however, is to make sure we handle memory we ourselves borrow externally accordingly. Which, we probably should, but is an entirely different topic.
We are only releasing the ones we allocated, not existing ones. Everyone can tell whether they own a specific one or not. First, because well, they are the one
Hmm. Did you even test? It feel like you based this example on assumptions, because well, It won't fail. Into
I'm not entirely sure what you are trying to point out here, but I assume that last line? If so, then it has nothing to do with us. Even without SP involved, if you load SM that way, the I have no idea why, nor do I care honestly, but yeah, it have nothing to do with us whatsoever.
There will always be clashes involved whenever we try to eat into each other's plate. This is not new. At the end of the day, this is the responsibility of server owners to make sure the plugins they install are compatible with each others. Sure, we are making effort. However, trying to predict what others may or may not do would never end. I personally didn't find your examples to be very convincing, and I still hold the same position on the topic as I did in my previous post. Just like everything else, if you dig long enough, you will find something. Perhaps even a treasure, who knows.
Because as previously stated, I don't see any valid reason to prevent plugins to load in such cases. To me, raising exceptions "is" what would introduce conflicting situations. |
|
I created a new issue #430 because this problem is not related to this pull request. I will also create an issue about ConCommand and ConVar conflicts, but that will come later.
The reason ConVar is being released is because SourceMod is releasing it intentionally. As I've said many times before, this problem has nothing to do with UnregisterConCommand.
First of all, unloading VSP/Meta Plugins and unloading SourcePython plugins should not be discussed in the same way. And this is bad design and should not be done.
You are not understanding the root of the problem, and you are missing the perspective from the plugin side. Code: from cvars import ConVar
from cvars import cvar
var = cvar.find_var("test_convar")
if var is not None:
cvar.unregister_base(var)
test_convar = ConVar("test_convar", "test", "Test Convar.") |
We should rather keep all the related discussions into the same thread. Multiple issues/PRs will only promote cross-referencing each others as well as duplicated posts iterating the same points and whatnot.
You know, if you re-read the thread, you will realize that the only reason why I mentioned a hook on
Then, you counter-argued my example stating that, although they are being unregistered, it doesn't mean they are freed. Which, fair enough, so I provided a hook on their destructor to further show you that they are being unregistered because they are being released. Now, you backtrack saying they are being released but still argue the examples I provided to debunk your original claim so I'm a bit confused as to whether I'm completely misinterpreting what you are trying to say.
And they should not have to. As previously mentioned, this is their responsibility to handle memory they don't own accordingly just like it is our to do so:
In fact, SM is listening for unregistrations and is internally unlinking their references so that they don't manipulate unregistered instances they don't own. We should probably implement something similar, but again, purposely leaking memory will never be the solution in my book.
I understand the issue perfectly. I even acknowledged it and stated that we should address it if you scroll up a few posts:
In any case, any issues you can possibly bring up regarding To be fair, I don't mind whatever. I said everything I had to say, and made my position on the topic very clear so I'm not sure what else can be said from my end. You guys can go about it whatever way you agree on. |
There is already
OnConVarChanged, but it is wasteful if you only want to know the changes of a specific ConVar.This was made to match SourceMod's AddChangeHook feature.
https://wiki.alliedmods.net/ConVars_(SourceMod_Scripting)#Change_Callbacks
Code:
Output: