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

DCS Continued #29324

Merged
merged 7 commits into from Jul 24, 2017

Conversation

Projects
None yet
@Cyberboss
Copy link
Member

commented Jul 17, 2017

I hate git sometimes

Closes #29317

This PR adds documentation and final touches missing from #29178

Datum Component System (DCS)

Concept

Loosely adapted from /vg/. This is an entity component system for adding behaviours to datums when inheritance doesn't quite cut it. By using signals and events instead of direct inheritance, you can inject behaviours without hacky overloads. It requires a different method of thinking, but is not hard to use correctly. If a behaviour can have application across more than one thing. Make it generic, make it a component. Atom/mob/obj event? Give it a signal, and forward it's arguments with a SendSignal() call. Now every component that want's to can also know about this happening.

In the code

Slippery things

At the time of this writing, every object that is slippery overrides atom/Crossed does some checks, then slips the mob. Instead of all those Crossed overrides they could add a slippery component to all these objects. And have the checks in one proc that is run by the Crossed event

Powercells

A lot of objects have powercells. The get_cell() proc was added to give generic access to the cell var if it had one. This is just a specific use case of GetComponent()

Radios

The radio object as it is should not exist, given that more things use the concept of radios rather than the object itself. The actual function of the radio can exist in a component which all the things that use it (Request consoles, actual radios, the SM shard) can add to themselves.

Standos

Stands have a lot of procs which mimic mob procs. Rather than inserting hooks for all these procs in overrides, the same can be accomplished with signals

API

Vars

  1. /datum/var/list/datum_components (private)
    • Lazy list of all components a datum has (TODO: Make this a typecache with longer paths overwriting shorter ones maybe? It'd be weird)
  2. /datum/component/var/enabled (protected, boolean)
    • If the component is enabled. If not, it will not react to signals
    • TRUE by default
  3. /datum/component/var/dupe_mode (protected, enum)
    • How multiple components of the exact same type are handled when added to the datum.
      • COMPONENT_DUPE_HIGHLANDER (default): Old component will be deleted, new component will first have /datum/component/proc/InheritComponent(datum/component/old, FALSE) on it
      • COMPONENT_DUPE_ALLOWED: The components will be treated as seperate, GetComponent() will return the first added
      • COMPONENT_DUPE_UNIQUE: New component will be deleted, old component will first have /datum/component/proc/InheritComponent(datum/component/new, TRUE) on it
  4. /datum/component/var/list/signal_procs (private)
    • Associated lazy list of signals -> callbacks that will be run when the parent datum recieves that signal
  5. /datum/component/var/datum/parent (protected, read-only)
    • The datum this component belongs to

Procs

  1. /datum/proc/GetComponent(component_type(type)) -> datum/component? (public, final)
    • Returns a reference to a component of component_type if it exists in the datum, null otherwise
  2. /datum/proc/GetComponents(component_type(type)) -> list (public, final)
    • Returns a list of references to all components of component_type that exist in the datum
  3. /datum/proc/GetExactComponent(component_type(type)) -> datum/component? (public, final)
    • Returns a reference to a component whose type MATCHES component_type if that component exists in the datum, null otherwise
  4. GET_COMPONENT(varname, component_type) OR GET_COMPONENT_FROM(varname, component_type, src)
    • Shorthand for var/component_type/varname = src.GetComponent(component_type)
  5. /datum/proc/AddComponent(component_type(type), ...) -> datum/component (public, final)
    • Creates an instance of component_type in the datum and passes ... to it's New() call
    • Sends the COMSIG_COMPONENT_ADDED signal to the datum
    • All components a datum owns are deleted with the datum
    • Returns the component that was created. Or the old component in a dupe situation where COMPONENT_DUPE_UNIQUE was set
  6. /datum/proc/ComponentActivated(datum/component/C) (abstract)
    • Called on a component's parent after a signal recieved causes it to activate. src is the parameter
    • Will only be called if a component's callback returns TRUE
  7. /datum/proc/TakeComponent(datum/component/C) (public, final)
    • Properly transfers ownership of a component from one datum to another
    • Singals COMSIG_COMPONENT_REMOVING on the parent
    • Called on the datum you want to own the component with another datum's component
  8. /datum/proc/SendSignal(signal, ...) (public, final)
    • Call to send a signal to the components of the target datum
    • Extra arguments are to be specified in the signal definition
  9. /datum/component/New(datum/parent, ...) (protected, virtual)
    • Forwarded the arguments from AddComponent()
  10. /datum/component/Destroy() (virtual)
    • Sends the COMSIG_COMPONENT_REMOVING signal to the parent datum if the parent isn't being qdeleted
    • Properly removes the component from parent and cleans up references
  11. /datum/component/proc/InheritComponent(datum/component/C, i_am_original(boolean)) (abstract)
    • Called on a component when a component of the same type was added to the same parent
    • See /datum/component/var/dupe_mode
    • C's type will always be the same of the called component
  12. /datum/component/proc/OnTransfer(datum/new_parent) (abstract)
    • Called before the new parent is assigned in TakeComponent(), after the remove signal, before the added signal
    • Allows the component to react to ownership transfers
  13. /datum/component/proc/_RemoveNoSignal() (private, final)
    • Internal, clears the parent var and removes the component from the parents component list
  14. /datum/component/proc/RegisterSignal(signal(string), proc_ref(type), override(boolean)) (protected, final) (Consider removing for performance gainz)
    • Makes a component listen for the specified signal on it's parent datum.
    • When that signal is recieved proc_ref will be called on the component, along with associated arguments
    • Example proc ref: .proc/OnEvent
    • If a previous registration is overwritten by the call, a runtime occurs. Setting override to TRUE prevents this
    • These callbacks run asyncronously
    • Returning TRUE from these callbacks will trigger a TRUE return from the SendSignal() that initiated it
  15. /datum/component/proc/ReceiveSignal(signal, ...) (virtual)
    • Called when a component recieves any signal and is enabled
    • Default implementation looks if the signal is registered and runs the appropriate proc

See signals and their arguments in __DEFINES\components.dm

Examples

Material Containers: #29268 (Too many GetComponent calls, but not bad)
Slips: #29339
Powercells: (TODO)
@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

I'll open up a new revert pr once this is merged, this doesn't resolve the issue.

@Cyberboss Cyberboss force-pushed the Cyberboss:components_doc branch from 17994a8 to e096f3f Jul 17, 2017

@Cyberboss Cyberboss force-pushed the Cyberboss:components_doc branch from e096f3f to a82e383 Jul 17, 2017


#### Slippery things

At the time of this writing, every object that is slippery overrides atom/Crossed does some checks, then slips the mob. Instead of all those Crossed overrides they could add a slippery component to all these objects. And have the checks in one proc that is run by the Crossed event

This comment has been minimized.

Copy link
@Jalleo

Jalleo Jul 17, 2017

Contributor

This is better to talk and collaborate about as a project and work on it rather than leaving to dust away here.

This comment has been minimized.

Copy link
@Supermichael777

Supermichael777 Jul 18, 2017

Contributor

implement this to show how its done then list it step by step as a practical example

This comment has been minimized.

Copy link
@Cyberboss
@ghost

This comment has been minimized.

Copy link

commented Jul 18, 2017

Actually, HOLY CRAB
That's obvious we need to document EVERY COMPONENT since the majority of them meant to be reused! It should be forced on contributors like changelog, maybe automated same way.

Example:
It would be fucking disgusting if someone ever create one component instead of two existing which do the same thing combined and start fucking slippery slope with that.

* Forwarded the arguments from `AddComponent()`
1. `/datum/component/Destroy()` (private)
* Use `RemoveComponent()` instead of `qdel()` to properly delete a component
* `qdel()` still works but it suppresses the `COMSIG_COMPONENT_REMOVING` signal and is not intended behavior

This comment has been minimized.

Copy link
@MrPerson

MrPerson Jul 18, 2017

Contributor

This is frankly incorrect. You should fix this behavior to be more standard with everything else ie qdel() to delete it.

This comment has been minimized.

Copy link
@Cyberboss

Cyberboss Jul 18, 2017

Author Member

Yeah, I'm trying to figure out how without adding another var, but it seems unlikely

Cyberboss added some commits Jul 18, 2017

fix

@Cyberboss Cyberboss force-pushed the Cyberboss:components_doc branch from a049d66 to 44f66ae Jul 18, 2017

@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

Its not documentation I want @Iamgoofball, it's that the original PR did not describe what the pr did in the pr body.

Pretending this is a solution is part of the problem.

I want cyberboss to:

  1. Edit a proper description into that pr,

  2. Add a proper description to this pr.

  3. Acknowledge that he messed up by not doing so.

  4. Apologize for not doing so

  5. State he will make a concerted effort to give better descriptions in the future.

@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

This isn't about the past, this is about the future, and the fact that @Cyberboss seems to think slapping some readme document in the repo is a adequate replacement for the basic requirement that you DETAIL WHAT THE FUCK YOUR PR IS tells me nothing has been learned, and this issue will come up in the future if nothing is done to change it.

@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

like @Iamgoofball, this pr itself doesn't even have a fucking description.

Like that makes my point clearer than anything else could.

@Cyberboss

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

Updated, happy now?

@Cyberboss

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

Ugh too much drama nevermind

@Cyberboss Cyberboss closed this Jul 19, 2017

@ghost

This comment has been minimized.

Copy link

commented Jul 19, 2017

@MrStonedOne

Acknowledge that he messed up by not doing so.
Apologize for not doing so
State he will make a concerted effort to give better descriptions in the future

Wow this amazing managment over VOLONTEER team, isn't it?

@Cyberboss Cyberboss deleted the Cyberboss:components_doc branch Jul 19, 2017

@TeknoKot

This comment has been minimized.

Copy link

commented Jul 19, 2017

ONE NAME ABOVE ALL, ALL OTHERS WILL FALL

@PJB3005

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

Honestly this just seems like mso doubling down instead of being reasonable again. Happens a lot.

TG you genuinely need to get yourself together. I'm not kidding when I say your codebase is a joke. This is just sad.

@Cyberboss Cyberboss restored the Cyberboss:components_doc branch Jul 19, 2017

@Cyberboss

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

@PJB3005 convinced me

@Cyberboss Cyberboss reopened this Jul 19, 2017

@Qustinnus

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

god bless the queen

@ghost

This comment has been minimized.

Copy link

commented Jul 20, 2017

can you get a normal tet-a-tet conversation with Cyberboss to see why you both misunderstanding each other so much?

@d3athrow

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

porting vg code

How dare you

@PJB3005

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@ghost

This comment has been minimized.

Copy link

commented Jul 20, 2017

  • Edit a proper description into that pr,
  • Add a proper description to this pr.
  • Acknowledge that he messed up by not doing so.
  • Apologize for not doing so
  • State he will make a concerted effort to give better descriptions in the future

image

@dannno

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

haha whatta fuck is this, man

can you cool it you fucking idiots

@Wyzack

This comment has been minimized.

Copy link

commented Jul 20, 2017

Is making him lick your boots and apologize really necessary?

@dannno

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

It will never be necessary and it's a complete embarrassment that he's trying to demand it to begin with

@Cyberboss

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

The previous PR's description has been updated and that's all I'm doing

@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

You still need to document this pr properly.

Something along the lines of Adds the documentation that was missing as well as something something bug fix something something qdel.

And I still want to see at least point 3 addressed. You've been scapegoating this entire thing, so I want to at least see you admit or acknowledge fault. even if it's just a simple my bad.

@ChangelingRain

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

I'm getting kind of tempted to just merge this and accelerate the drama, since it's a perfectly functional pr and system being helped back by someone wanting the author to acknowledge fault.

@MrStonedOne

This comment has been minimized.

Copy link
Member

commented Jul 23, 2017

This PR still has no valid description describing what it does. The description provided is for another PR, and this one has changes that aren't documented

@optimumtact

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

lol what is going on here

@Qustinnus

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

only god knows

@optimumtact

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

we have a private irc channel?

@optimumtact

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

I ticked all 5 of MSO's boxes so I can merge this now

@optimumtact optimumtact merged commit 0321e6b into tgstation:master Jul 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@FTL13-Bot FTL13-Bot referenced this pull request Jul 24, 2017

Closed

[MIRROR] DCS Continued #1700

HonkBot pushed a commit to HippieStation/HippieStation that referenced this pull request Jul 24, 2017

DCS Continued (tgstation#29324)
Adds's documentation to the DCS system, refactors to improve the caller API

@HonkBot HonkBot referenced this pull request Jul 24, 2017

Merged

[MIRROR] DCS Continued #2505

MrStonedOne added a commit to MrStonedOne/tgstation that referenced this pull request Jul 24, 2017

MrStonedOne added a commit to MrStonedOne/tgstation that referenced this pull request Jul 24, 2017

tortellinitony added a commit to HippieStation/HippieStation that referenced this pull request Jul 24, 2017

[MIRROR] DCS Continued (#2505)
* DCS Continued (tgstation#29324)

Adds's documentation to the DCS system, refactors to improve the caller API

* DCS Continued

@Cyberboss Cyberboss deleted the Cyberboss:components_doc branch Jul 26, 2017

@Qustinnus Qustinnus referenced this pull request Aug 8, 2017

Merged

[MIRROR] DCS Continued #1894

@Time-Green Time-Green referenced this pull request Jan 12, 2018

Merged

Datum Component System #2932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.