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

[MIRROR] Remove data systems in favor of global datums #3109

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

Steals-The-PRs
Copy link
Collaborator

Mirrored on Nova: NovaSector/NovaSector#2242
Original PR: tgstation/tgstation#82943

About The Pull Request

Removes data systems in favor of global datums. After merge I want to add a contribution guideline in favor of global datums rather than subsystems with no behavior since that seems to be something we don't want.

CC jlsnow301

Why?

The original motivation of data systems is to replace subsystems that don't fire or tick. I don't necessarily know that I agree that that's an issue, but there's enough push in that direction and I don't disagree with it strongly enough to care about it.

However, I am very much against treating these as some special bespoke concept worthy of its own abstraction. I do not see it as providing any value and only adds to confuse.

We can start by acknowledging the types of persistent global controllers/datums we have today:

  1. Things that need to repeatedly do something, often statefully. Ex: radiation, which looks for sources, and then acts on nearby targets.
  2. Things that just act as a namespace, and are completely stateless. Ex: the move manager, which contains helper utilities for moving based on a subsystem's tickrate.
  3. Things that are not stateless, but do not "think". Ex: the materials cache, which must initialize globally and reuse values from the cache, but also will never do anything unless explicitly prodded.
  4. Things that are not stateless, and behave autonomously. Ex: highlander, which does not exist until necessary, but also registers several signals and once initialized, handles itself.

The behavior of 1 has proven worthy of its own abstraction, as doing so allows us greater control through the master controller. 2, 3, and 4, have NOT, and data systems do not replace them entirely. As an easy callout, data systems are not meant to do any interesting initialize behavior, so the 4th use case is immediately out, meaning things like the highlander controller have to remain in global datums.

But this cost does have a burden on the developer, as there are now 3 unique namespaces (an ACTUAL downside to "namespace pollution") that a mechanic they want to use might be in. What makes "materials" something that obviously isn't a subsystem, and is a data system? What makes highlander something that obviously isn't a subsystem or data system, but instead a global datum?

It also carries a similar problem to components vs. elements, which is that your implementation details are leaked to consumers, and changing those implementation details can carry a heavy cost on your PR. Data systems are not meant to have interesting initialization, but eventually if something like radio does, then now we either have to break that rule for DSradio (which this PR does), or we have to change it to another contraption and change every reference of DSradio to whatever it is now. I would like to reduce that where necessary.

So the new rule that I will propose in contribution guidelines later is simple, assuming we agree that NO_FIRE|NO_INIT subsystems are a smell--subsystems if they do something repeatedly and unconditionally, probably global datums otherwise.

* Remove data systems in favor of global datums (#82943)

* Remove data systems in favor of global datums

* Update ammo_workbench.dm

---------

Co-authored-by: Mothblocks <35135081+Mothblocks@users.noreply.github.com>
Co-authored-by: Mal <13398309+vinylspiders@users.noreply.github.com>
@mogeoko mogeoko merged commit 5408144 into master Apr 30, 2024
26 checks passed
@mogeoko mogeoko deleted the upstream-mirror-2242 branch April 30, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants