metauni / metaboard

Multiplayer drawing boards for sharing knowledge in Roblox.
Mozilla Public License 2.0
28 stars 6 forks source link

Board state in Code, BaseValues and Attributes best practice #71

Closed blinkybool closed 1 year ago

blinkybool commented 1 year ago

In the Board Instance section of the CONTRIBUTING document, I wrote this

Note that having too many of these kinds of methods should be considered an anti-pattern, especially getter and setter methods. Instead of Board:GetX() and Board:SetX(newX), it's better to just have X as a key in the board table. If there is a behaviour that is supposed to be triggered when the X key changes, then make a simple interface for external code to manually trigger that behaviour after modifying X. An example of this is the Loaded key and the LoadedSignal in BoardServer, which should be fired whenever board.Loaded becomes true. We could make a :GetLoaded() and :SetLoaded(isLoaded) method, where the SetLoaded method also fires the signal, with the intention of keeping that behaviour a responsibility of the board object, but this just obscures what's going on to the caller of SetLoaded and introduces two extra methods. Better to just write these two lines of code each time.

board.Loaded = true
board.LoadedSignal:Fire()

The LoadedSignal comes from the "GoodSignal" code made by stravant. Read here.

I've since realised that this might be better solved by using BoolValue parented to the board instance (called Loaded), or an Attribute.

  1. The signal comes for free with the value
  2. The signal is fired automatically when the value is changed
  3. The state of the value is replicated automatically from server to clients.
  4. Using a BoolValue (or an Attribute) provides an easy public interface for external code to react to the state of the board.

(3) and (4) seem to be the most important benefits.

This got me thinking in general about the ideal source of truth for the board state. Clearly figures, drawing tasks, and player histories are too complex and slow to store in the data model, and so putting them in the associated Board object has many speed and flexibility benefits. However it then loses out on (1) to (4).

There are a few discussions I've found (like [this](https://devforum.roblox.com/t/from-an-efficiencyeffectivenessnot-convenience-standpoint-are-objectvalues-better-than-passing-data-through-remotes/85053/7 and this -- see MrChickenRocket's reply) about pros and cons of using BaseValues, Attributes vs other approaches, regarding how they behave w.r.t. replication and firing changed events. I think the best approach is to consider each property separately, and decide whether it's more appropriate to put it in the data model or in the Board object. Things to keep in mind are

  1. How often does the value change?
  2. How quickly should must other code react to this change?
  3. Does the value get updated by the server or client?

Answers more like "less often, not very quickly, just the server" I think steer towards using Attributes (or BaseValues), rather than using a key in the Board table and keeping them in sync with remote events.

For example I think using Loaded as an attribute of the board instance would be sensible, since it only changes once, when the server loads the board from persistence (though I would probably change it to ServerLoaded for clarity).

Another example is for the admin permissions. In metaboardv0, you need to get the remoteEvent (which involves a WaitForChild) request the state from the server on startup and then store it in CanvasState.HasWritePermission, then when it changes, store the new value and react to it changing (if applicable). If this state was stored somehow with BaseValues or Attributes, you can just immediately grab whatever the value is (who cares if MetaAdmin exists), and connect to the signal to react when the server changes it.

Another domain that I think we should consider how/whether we use Attributes/BaseValues is in configuration inputs to metaboard/metaorb/metapocket etc. We have a Config file in all of these, but there are some use cases where you might want different configurations for different places. I don't think editing this (and re-editing when there are new releases) is a reasonable expectation to solve this. Instead, we could allow the user to give attributes to ServerScriptService/Workspace/ReplicatedStorage or parent BaseValues to them, in order to specify things like the name of the datastore to use for persistence.

Btw we already do this for each of objects of concern with these modules (like persistIds on metaboards).

blinkybool commented 1 year ago

Good thoughts. No need to keep the issue open for now.