Changing an agent's visualization properties on run time fails

[jwaa]

Describe the bug
At times you want to change a visualization property such as the size, color or image of an agent. This however is currently impossible to do in a clear way. As it requires you to know that when MATRX checks for changes in the visualization properties it goes from this naming scheme properties['visualization']['size'] to this scheme properties['visualization_size']. This is first of all far from clear that this needs to be done, and if it is then it doesn't work.

Even when a user knows this and thus changes the property with the more flat naming schemen (i.e. properties['visualization_size']), it fails. As here is a failsafe built in to check if the property is present, which does not account for this more flat naming scheme. Thus it raises the exception.

To Reproduce

from matrx import WorldBuilder
from matrx.agents import AgentBrain


class TestBrain(AgentBrain):

    def __init__(self):
        super().__init__()

    def decide_on_action(self, state):
        self.agent_properties['visualize']['size'] = 2
        return None, None


def test_builder(run_matrx_api, run_matrx_visualizer, tick_speed):
    builder = WorldBuilder(shape=[3, 3], run_matrx_api=run_matrx_api, run_matrx_visualizer=run_matrx_visualizer,
                           visualization_bg_img='', tick_duration=tick_speed,
                           simulation_goal=500)
    builder.add_agent(name="Test", agent_brain=TestBrain(), location=(1,1))

    return builder


if __name__ == "__main__":
    builder = test_builder(run_matrx_api=True, run_matrx_visualizer=True, tick_speed=1)
    builder.startup()
    world = builder.get_world()
    world.run(api_info=builder.api_info)
    builder.stop()

The "fix" where the property name is flat, also fails. Can be tested by chaning this line in the above snippet:

self.agent_properties['visualize']['size'] = 2

with this line:

self.agent_properties['visualize_size'] = 2

Expected behavior
A change in the visualization property as expected and during run time.

Screenshots
N/A

Stacktrace
The first snippet gives this trace:

Traceback (most recent call last):
  File ".../matrx_test/change_property_test.py", line 28, in <module>
    world.run(api_info=builder.api_info)
  File "...\matrx-test\lib\site-packages\matrx\grid_world.py", line 246, in run
    is_done, tick_duration = self.__step()
  File "...\matrx-test\lib\site-packages\matrx\grid_world.py", line 699, in __step
    agent_obj._set_agent_changed_properties(agent_properties)
  File "...\matrx-test\lib\site-packages\matrx\objects\agent_body.py", line 241, in _set_agent_changed_properties
    self.change_property(prop, props[prop])
  File "...\matrx-test\lib\site-packages\matrx\objects\agent_body.py", line 313, in change_property
    raise Exception(f"Couldn't change property {property_name} for object with ID {self.obj_id} as it doesn't exist (use `add_property()` instead) or isn't allowed to be changed.")
    
Exception: Couldn't change property visualization for object with ID test as it doesn't exist (use `add_property()` instead) or isn't allowed to be changed.

With the flat property name we get;

Traceback (most recent call last):
  File ".../matrx_test/change_property_test.py", line 28, in <module>
    world.run(api_info=builder.api_info)
  File "...\matrx-test\lib\site-packages\matrx\grid_world.py", line 246, in run
    is_done, tick_duration = self.__step()
  File "...\matrx-test\lib\site-packages\matrx\grid_world.py", line 699, in __step
    agent_obj._set_agent_changed_properties(agent_properties)
  File "...\matrx-test\lib\site-packages\matrx\objects\agent_body.py", line 233, in _set_agent_changed_properties
    raise Exception(f"Agent {self.obj_id} tried to remove the property {prop}, which is not allowed.")
Exception: Agent test tried to remove the property visualize_size, which is not allowed.

By the way, this exception makes no sense. It tells the user the agent tries to remove a property that is not present in its property list.

Additional context
See the issue #329 for its occurence, this took place in v2.2.0 where the "wrong" way (i.e. properties['visualize']['size'] = 5.0) did not raise an exception yet. However the "right" way also did not work there.

#325 and its fix seems related; why does it work for the visualization image but not for the size?

[jwaa]

A hacky solution is to change the statement on Line 229 in AgentObject to this:

# check for each property if it has been changed by the agent, and if we need
# to update our local copy (here in Agent's body) of the agent properties to match that
for prop in props.keys():
    # check if the property has changed, and skip if not the case
    if not str(prop).startswith("visualize") and props[prop] == body_properties[prop]:
        continue

    # The agent changed the property and the agent had permission to do so
    # update special properties
    self.change_property(prop, props[prop])

[thaije]

Hmm that is an ugly bug indeed. I also tested it and see no other workaround at the moment. The bug still occurs in MATRX 2.3.1.
I think a pretty solution would be to just flatten the agent properties to always use visualize_size, and not nested in a visualize parent. Why do we have to nesting anyway? 🤔
That would be a (minor) breaking change though, so officially according to semantic versioning that would mean MATRX v3..

This error does not occur for img_name as it is not a default property, but a custom property that changes some stuff in the frontend. img_name is also not nested. Preferably img_name would be added to EnvObject as a default property as well.

[jwaa]

Why do we have to nesting anyway? 🤔

Legacy code!

But yes, lets flatten that dictionary. It will be a bit of an ordeal, as with any legacy code; it is everywhere! I think we may even need to make some changes to the visualization... So in that sense we may turn out with a v3.0!

Though I think we don't have to take it that far. We can limit the fix at this point to the hacky solution I added with some additional deprecation messages for functions that assumes a non-flat properties dictionary. That makes it a very hacky and temporary patch, but a working one (and let's not forget to make an issue then to really fix it...).