Add Core:setStateComponent and Core:setStateSingleton #42

benbrimeyer · 2020-02-25T06:14:23Z

Adds the following members to Core to support a setStateComponent method

Core:setStateSingleton(componentId, newState)
- Used to set the component's state of a singleton

Given a a component identifier and a dictionary of fields and values, overwrites the state of an existing singleton. Returns a boolean that is true if the component's state was changed. Throws if the identified singleton has not been added to the entity or if the identified component class isn't registered in the Core.

Core:setStateComponent(entityId, componentId, newState)
- Used to set the component's state of an entity

Given an entity ID, a component identifier, and a dictionary of fields and values, overwrites the state of an existing component of the entity. Returns a boolean that is true if the component's state was changed. Throws if the identified component class has not been added to the entity or if the identified component class isn't registered in the Core.

Core:getComponentStateSetSignal(componentId)
- Fires when setStateComponent is called on an entity

Gets a signal that fires whenever a component's state is set. The signal will be fired with the entity ID and the component that was set. Throws if the identified component class isn't registered in the Core.

Core.None
- When used with a new state of setStateComponent, will delete the value from the component's state.

A symbol that can be used in conjunction with Core:setStateComponent method which will tell the Core to delete a field of a component if used as part of the new given state.

Relevant core plugin lifecycle added as well: componentStateSet(entityId, componentInstance)

AmaranthineCodices

Sorry for taking so long to review this 😢

There's a couple of changes I'd like to see made here before merging.

AmaranthineCodices · 2020-05-03T08:09:58Z

src/Core.lua

+    or if the identified component class isn't registered in the Core.
+
+]]
+function Core:setStateComponent(entityId, componentIdentifier, newState)


I think I'd prefer setComponentState here, I think it reads better!

I opted for setStateComponent to keep in the tradition of [actionType]Component as that also translates well to the plugin variants.

AmaranthineCodices · 2020-05-03T08:10:28Z

src/Core.lua

+        local componentInstance = componentInstances[entityId]
+
+        if componentInstance ~= nil then
+


Prefix newline here can be removed.

Suggested change

AmaranthineCodices · 2020-05-03T08:13:24Z

src/Core.spec/setStateComponent.spec.lua

+            core:setStateComponent(entity, ComponentClass, { x = 0 })
+        end).to.throw()
+    end)
+


Suggested change

AmaranthineCodices · 2020-05-03T08:14:21Z

src/Core.spec/setStateComponent.spec.lua

+
+        local entity = core:createEntity()
+        core:addComponent(entity, ComponentClass)
+        core:setStateComponent(entity, ComponentClass, { x = 10, y = 20 })


This doesn't actually verify that the fields were set here, just that the call didn't throw.

AmaranthineCodices · 2020-05-03T08:18:56Z

src/Core.lua

+
+    Given an entity ID, a component identifier, and a dictionary of fields
+    and values, overwrites the state of an existing component of the entity.
+    Returns a boolean that is true if the component's state was changed.


This method doesn't actually return false in any case. It'd make sense to remove this or change the implementation; a good place to check for a possible diff would be in the loop over newState.

It would also make sense to not fire the signal/invoke the plugin methods if we know a mutation didn't take place.

AmaranthineCodices · 2020-05-03T08:20:16Z

src/Core.lua

+        end
+    else
+        error(errorMessages.componentNotRegistered:format(componentIdentifier), 2)
+    end


We should return false, nil here to avoid implicitly returning 0 values.

AmaranthineCodices · 2020-05-03T08:21:45Z

src/Core.spec/setStateComponent.spec.lua

+
+        local entity = core:createEntity()
+        core:addComponent(entity, ComponentClass)
+        local success, component = core:setStateComponent(entity, ComponentClass, { x = 10, y = core.None })


success here is a misnomer: the first return value indicates state mutation, not success. This should be probably named mutated or something similar.

AmaranthineCodices · 2020-05-03T08:23:28Z

src/Core.spec/setStateComponent.spec.lua

+        expect(component.x).to.equal(10)
+        expect(component.y).to.equal(20)
+    end)
+


I'd like to see an explicit test for the first return value, including the cases where it will be false (setState on an empty table and with the same values, etc).

AmaranthineCodices · 2020-05-03T08:27:58Z

src/Core.lua

@@ -24,6 +24,9 @@
    - componentAdded(Core, entityId, componentInstance): Called when a component
      instance is added to an entity. Called before the addition signal for that
      component has been fired.
+      componentStateSet(Core, entityId, componentInstance): Called when a component


Would it make sense for plugins to be able to observe the state diff? This would be useful for e.g. replication plugins so that they can send just the state diff rather than the entire component state.

* master: update rotriever.toml

…entState

Phalanxia · 2020-05-25T07:38:33Z

What are some use cases you have in mind for this? 🙂

benbrimeyer added 4 commits February 24, 2020 22:04

Basics working

d3cf271

Added Core.None

7cee6a0

Add comment for Core.None

fc52c1f

fix minor typo "conjunction"

42ce281

benbrimeyer requested a review from AmaranthineCodices February 25, 2020 06:14

indentation tabs > spaces

3e542a8

benbrimeyer mentioned this pull request Feb 26, 2020

DEMO: Allow Cores to replicate setStateComponent call vocksel/recs-replication-plugin#3

Open

benbrimeyer added 2 commits April 22, 2020 15:09

update rotriever.toml

67208a4

add component identifier to state set plugin lifecylce

524fd85

AmaranthineCodices requested changes May 3, 2020

View reviewed changes

benbrimeyer added 3 commits May 20, 2020 17:33

Merge branch 'master' into feature/setComponentState

1ed424d

* master: update rotriever.toml

Fix broken setStateComponent test

b24118d

Add setStateSingleton and plugin method

d9d50fe

benbrimeyer changed the title ~~Add Core:setStateComponent~~ Add Core:setStateComponent and Core:setStateSingleton May 21, 2020

benbrimeyer added 2 commits May 20, 2020 18:04

Update rotriever lock

c4dce55

Merge remote-tracking branch 'upstream/master' into feature/setCompon…

4c8942d

…entState

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Core:setStateComponent and Core:setStateSingleton #42

Add Core:setStateComponent and Core:setStateSingleton #42

benbrimeyer commented Feb 25, 2020 •

edited

Loading

AmaranthineCodices left a comment

AmaranthineCodices May 3, 2020

benbrimeyer May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

AmaranthineCodices May 3, 2020

Phalanxia commented May 25, 2020

		local componentInstance = componentInstances[entityId]

		if componentInstance ~= nil then

Add Core:setStateComponent and Core:setStateSingleton #42

Are you sure you want to change the base?

Add Core:setStateComponent and Core:setStateSingleton #42

Conversation

benbrimeyer commented Feb 25, 2020 • edited Loading

AmaranthineCodices left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phalanxia commented May 25, 2020

benbrimeyer commented Feb 25, 2020 •

edited

Loading