Call NetworkVarNotify functions AFTER variable is set

[WilliamVenner]

Details

Because NetworkVarNotify is called before the actual variable (returned by the getter) is set, it is extremely tedious to do any "dynamic behaviour" inside one of these callbacks.

Instead, one has to store an internal variable, like self.m_VariableName with the new value, to call any functions of the entity that are NOT the NetworkVarNotify callback, but need the data that was changed.

I don't think changing this could break anything. If it does then it would make working with this x100 easier if "post-callbacks" were a separate feature.

[robotboy655]

You mean this? https://wiki.facepunch.com/gmod/Entity:NetworkVarNotify

You do realize the callback has arguments for both new and old value?

[WilliamVenner]

Problem

function ENT:MyVarChanged(name, oldVal, newVal)
    self:SomeArbritraryOperationThatUsesMyVar() -- prints oldVal
end

function ENT:SomeArbritraryOperationThatUsesMyVar()
    print("Value of MyVar: " .. self:GetMyVar()) -- could print oldVal or newVal
end

function ENT:Touch() -- could be any event
    self:SomeArbritraryOperationThatUsesMyVar() -- prints newVal
end

"""Solution"""

function ENT:MyVarChanged(name, oldVal, newVal)
    self.m_MyVar = newVal
    self:SomeArbritraryOperationThatUsesMyVar() -- prints newVal
end

function ENT:SomeArbritraryOperationThatUsesMyVar()
    print("Value of MyVar: " .. self.m_MyVar) -- prints newVal
end

function ENT:Touch() -- could be any event
    self:SomeArbritraryOperationThatUsesMyVar() -- prints newVal
end

This is not ideal

[WilliamVenner]

https://github.com/Facepunch/garrysmod/blob/8a4c61dec895a100bc88dc95af82856743a76f83/garrysmod/lua/includes/extensions/entity.lua#L296-L297

[robotboy655]

Solution by a sane person:

function ENT:MyVarChanged(name, oldVal, newVal)
    self:SomeArbritraryOperationThatUsesMyVar( newVal )
end

function ENT:SomeArbritraryOperationThatUsesMyVar( var )
    print("Value of MyVar: " .. var ) -- prints newVal
end

function ENT:Touch()
    self:SomeArbritraryOperationThatUsesMyVar( self:GetThatVariable() )
end

[WilliamVenner]

Does this look sane to you?

function ENT:MyVar1Changed(name, oldVal, newVal)
    self:SomeArbritraryOperationThatUsesMyVars(newVal, self:GetMyVar2(), self:GetMyVar3() )
end

function ENT:MyVar2Changed(name, oldVal, newVal)
    self:SomeArbritraryOperationThatUsesMyVars( self:GetMyVar1(), newVal, self:GetMyVar3() )
end

function ENT:MyVar3Changed(name, oldVal, newVal)
    self:SomeArbritraryOperationThatUsesMyVars( self:GetMyVar1(), self:GetMyVar2(), newVal )
end

function ENT:SomeArbritraryOperationThatUsesMyVars( Var1, Var2, Var3 )
    print( Var1, Var2, Var3 )
end

function ENT:Touch()
    self:SomeArbritraryOperationThatUsesMyVar( self:GetMyVar1(), self:GetMyVar2(), self:GetMyVar3() )
end

Versus

function ENT:SetupDataTables()
    ... blah ...

    self:NetworkVarNotify("MyVar1", self.SomeArbritraryOperationThatUsesMyVars)
    self:NetworkVarNotify("MyVar2", self.SomeArbritraryOperationThatUsesMyVars)
    self:NetworkVarNotify("MyVar3", self.SomeArbritraryOperationThatUsesMyVars)
end

function ENT:SomeArbritraryOperationThatUsesMyVars()
    print( self:MyVar1(), self:MyVar2(), self:MyVar3() )
end

function ENT:Touch()
    self:SomeArbritraryOperationThatUsesMyVars()
end

????

[robotboy655]

I see your point, but you know mine, and I can't just take I don't think changing this could break anything and change this behavior.

Your use case is also very rare.

[WilliamVenner]

How about a third argument to NetworkVarNotify that gets called afterwards?

[robotboy655]

No, that's just stupid.

[WilliamVenner]

But so is the current behaviour!

Your use case is also very rare.

Are you sure about that?

https://github.com/Facepunch/garrysmod/blob/394ae745df8f8f353ea33c8780f012fc000f4f56/garrysmod/gamemodes/sandbox/entities/entities/edit_sun.lua#L53-L90

[WilliamVenner]

https://github.com/Facepunch/garrysmod/blob/9d90b5a014072e3a504bbeb69838921622bb0a6f/garrysmod/gamemodes/sandbox/entities/entities/gmod_lamp.lua#L129-L167

[robotboy655]

I meant using multiple variables in 1 function/callback like in your second example, not using the callbacks by themselves.

[WilliamVenner]

Those are using multiple variables in 1 function/callback.

[robotboy655]

I mean true, but they don't need to.

[WilliamVenner]

But it would look pretty stupid if they did need to, hence why they don't.

You realize that https://github.com/Facepunch/garrysmod/blob/394ae745df8f8f353ea33c8780f012fc000f4f56/garrysmod/gamemodes/sandbox/entities/entities/edit_sun.lua#L53-L90 is an actual bug, right?

[TiberiumFusion]

Your use case is also very rare.

Since when is it rare to expect event-driven code to operate in a sane way that is consistent with the superior pattern established by mature & ubiquitous platforms? The primary reason a callback should fire before the event completes is to provide a chance to suppress it, which NetworkVarNotify does not provide. Reading the property before the value is updated can certainly be useful, but there is absolutely no good reason to provide a "preview" event while simultaneously lacking a "normal" event which is raised after the property is updated - elsewise it is a preview to nothing.

• By all means, as suggested by WilliamVenner, NetworkVarNotify should be called after the property is updated with the new value.
• Or, NetworkVarNotify is renamed to PreviewNetworkVarNotify, and a new NetworkVarNotify callback is created which is called after the property updates. Both have the same parameters, and PreviewNetworkVarNotify will supply a means to suppress the later NetworkVarNotify event.
  • However, because NetworkVarNotify is a notification event, there is no good reason to suppress it. So, unless there is some crucial need for Glua to provide a preview event for network var updates (i.e. the current, unnatural behavior of NetworkVarNotify), this solution is excessive and NetworkVarNotify should simply be fixed to run after the property update.

[robotboy655]

I swear you are trying your best to misunderstand what I wrote.

What I wrote was having 3 interdependent network variables in network callbacks is quire rare in my option. None of the examples above are interdependent. I did not say using network events is rare. Which means you can easily get away with my initial example in 99% of cases. Sorry if that didn't across as what I intended. Not that it matters.

[WilliamVenner]

To be honest I would be happy with simply NetworkVarPostNotify

[robotboy655]

Made it call callbacks after the variable is set, lets see if this breaks shit.

[robotboy655]

And it breaks the default Lamp tool, which means it will break a bunch of mods as well.

[WilliamVenner]

What's broken about it?

[robotboy655]

https://github.com/Facepunch/garrysmod/blob/master/garrysmod/gamemodes/sandbox/entities/entities/gmod_lamp.lua#L132 https://github.com/Facepunch/garrysmod/blob/master/garrysmod/gamemodes/sandbox/entities/entities/gmod_lamp.lua#L87

Basically its core function breaks. I already fixed on Dev/x64 where NetworkVarNotify was changed, but this will most likely cause issues with other mods.

[WilliamVenner]

Guess we need a new function instead then.

[meepen]

The old and new variables passed to NetworkVarNotify are backwards.

function ENT:NetworkVarNotifyCallback(name, old, new)
    print(name, old, new)
    if (old == new) then
        return
    end
    hook.Run("On" .. name .. "Change", old, new)
end

[meepen]

newest update seems to fix that

[thegrb93]

This started happening after the Jan update I think.

[starfallex] addons/starfallex/lua/entities/starfall_hologram/cl_init.lua:28: bad argument #1 to 'Scale' (Vector expected, got nil)

Scale - [C]:-1 v - addons/starfallex/lua/entities/starfall_hologram/cl_init.lua:28

3. CallProxies - lua/includes/extensions/entity.lua:265
4. CallDTVarProxies - lua/includes/extensions/entity.lua:273
5. unknown - lua/includes/extensions/entity.lua:171 (x96)

https://github.com/thegrb93/StarfallEx/blob/master/lua/entities/starfall_hologram/cl_init.lua#L28

I'm guessing the newest update meepen mentioned already fixes it hopefully.

[robotboy655]

Let me know if it still happens on Monday or something, and make sure your server is up to date if it is on any beta branches.

Should give everyone enough time to get their clients updated on the Beta branches.

[robotboy655]

Had to reverse this change still, it just causes too many subtle issues with existing addons, my Lightsabers included.

And I do not like the idea of yet another function for something so simple, sigh...