[Bug fix] Navigation scenario: ensure entity placement within constrained environment boundaries

[Giovannibriglia]

It resolves Issue #138

Changes Made:

• Removed self.world_semidim.
• Introduced self.world_semidim_x and self.world_semidim_y, which default to 1 (as before). If the user opts for a limited-dimension environment, these variables are set to the values of self.x_semidim and self.y_semidim.

Enhancement:

• Ensured that all entities are correctly placed within the defined boundaries."

[matteobettini]

Cool! We should have caught this as part of #133, but I think it is ok since we didn't make a release in the mean time

[matteobettini]

Remove this

[Giovannibriglia]

Did I do it correctly?

[Giovannibriglia]

I noticed that in the discovery scenario, the implementation of the x and y bounds differs slightly from what I've done. Here's the relevant link:

VectorizedMultiAgentSimulator/vmas/scenarios/discovery.py

Line 136 in ff58363

x_bounds=(-self.world.x_semidim, self.world.x_semidim),

How would you suggest proceeding? In the navigation scenario, this pull request introduces two new variables, while in the discovery scenario, the world object is used instead.

[matteobettini]

I noticed that in the discovery scenario, the implementation of the x and y bounds differs slightly from what I've done. Here's the relevant link:

VectorizedMultiAgentSimulator/vmas/scenarios/discovery.py

Line 136 in ff58363

x_bounds=(-self.world.x_semidim, self.world.x_semidim),

How would you suggest proceeding? In the navigation scenario, this pull request introduces two new variables, while in the discovery scenario, the world object is used instead.

In discovery self.world.x_semidim is guaranteed to not be None, while here not. I think the current PR makes sense

[Giovannibriglia]

I noticed that in the discovery scenario, the implementation of the x and y bounds differs slightly from what I've done. Here's the relevant link:

VectorizedMultiAgentSimulator/vmas/scenarios/discovery.py

Line 136 in ff58363

x_bounds=(-self.world.x_semidim, self.world.x_semidim),

How would you suggest proceeding? In the navigation scenario, this pull request introduces two new variables, while in the discovery scenario, the world object is used instead.

In discovery self.world.x_semidim is guaranteed to not be None, while here not. I think the current PR makes sense

Great! Could you explain why the discovery scenario is boundary-limited, whereas navigation is not? What’s the reasoning behind this?

[matteobettini]

I am wondering about another thing.

If users want to specify the semidims for spawning entities but not have hard bounds, they currently can't.

What about:

• having 3 kwargs: the semidims ones that we already introduced, and a new one enfornce_bounds (defaults False)

So that:

• if none is specified it is like before -1,1 without bounds
• if the semidim are defined only, they regulate the range of spawning entities but not the world semidims
• if the semidim are defined and enfornce_bounds is true they regulate both the world bounds and the spawning bounds
• if enfornce_bounds is true and they do not give any semidims -> error

[matteobettini]

discovery scenario is boundary-limited, whereas navigation is not? What’s the reasoning behind this?

Just the way they were implemented, which we have to keep as default for bc-compatibility. Now we have the option of bounding navigation too so they are more similar, just with different defaults

[Giovannibriglia]

I am wondering about another thing.

If users want to specify the semidims for spawning entities but not have hard bounds, they currently can't.

What about:

• having 3 kwargs: the semidims ones that we already introduced, and a new one enfornce_bounds (defaults False)

So that:

• if none is specified it is like before -1,1 without bounds
• if the semidim are defined only, they regulate the range of spawning entities but not the world semidims
• if the semidim are defined and enfornce_bounds is true they regulate both the world bounds and the spawning bounds
• if enfornce_bounds is true and they do not give any semidims -> error

Which scenario are you referring to? I didn't find enforce_bounds variable :/

[matteobettini]

Which scenario are you referring to? I didn't find enforce_bounds variable :/

I am proposing to introduce it here

[Giovannibriglia]

Which scenario are you referring to? I didn't find enforce_bounds variable :/

I am proposing to introduce it here

Oh okay, now I got you. I believe this could be a valuable enhancement. If I understand correctly:

• semidim variables are used in the entity spawning process.
• enforce_bounds is used to set the environment's limits when the semidim variables are not None

Therefore:

• If both semidim and enforce_bounds are active: entities are spawned within the constrained world limits and the world is constrained too.
• If semidim is active but enforce_bounds is not: entities are spawned within semidim's limits, but the world is unlimited.
• If enforce_bounds is active but semidim is not: error occurs.
• Otherwise: the default range (-1, 1) is used for spawning, and the world is unlimited.

[matteobettini]

Oh okay, now I got you. I believe this could be a valuable enhancement. If I understand correctly:

• semidim variables are used in the entity spawning process.
• enforce_bounds is used to set the environment's limits when the semidim variables are not None

Therefore:

• If both semidim and enforce_bounds are active: entities are spawned within the constrained world limits and the world is constrained too.
• If semidim is active but enforce_bounds is not: entities are spawned within semidim's limits, but the world is unlimited.
• If enforce_bounds is active but semidim is not: error occurs.
• Otherwise: the default range (-1, 1) is used for spawning, and the world is unlimited.

Exactly!

Let’s also add this comments to the kwargs explaining what they do.

Also there is the edge case of only one of the semidims been specified. In the case this happens:

• if the world is bounded, we bound only the specified one
• For spawning we default to 1 for the other

[Giovannibriglia]

Hi :)

I've been considering the solution, but I've realized that it might lead to navigation behavior that's different from other scenarios. Let me explain:

• In our proposed solution, x_semidim and y_semidim were used as variables in the entity spawning process. However, in all other scenarios, they act as environment boundaries. Therefore, we should use these variables as follows:

self.world_spawning_x = kwargs.pop("x_semidim", None)
self.world_spawning_y = kwargs.pop("y_semidim", None)

• Following that, there's the initialization of enforce_bounds:

self.enforce_bounds = kwargs.pop("enforce_bounds", False)

I set this to False by default to maintain consistency with previous cases, but I'm open to any suggestions you might have regarding this.

• After developing this logic, here's the implementation I've come up with, which I believe aligns with our previous discussions:

if self.enforce_bounds:
    if self.world_spawning_x is None or self.world_spawning_y is None:
        raise ValueError("enforce_bounds is active, but one or both semidims are None.")
    self.x_semidim = self.world_spawning_x
    self.y_semidim = self.world_spawning_y
else:
    self.x_semidim = None
    self.y_semidim = None

• and I will use world_spawning_x and world_spawning_y for the entities process spawning in

  VectorizedMultiAgentSimulator/vmas/scenarios/navigation.py

  Line 133 in ff58363

  ScenarioUtils.spawn_entities_randomly(

  and

  VectorizedMultiAgentSimulator/vmas/scenarios/navigation.py

  Line 150 in ff58363

  position = ScenarioUtils.find_random_pos_for_entity(

I think this approach is fairly clear because x_semidim and y_semidim are used to initialize world_spawning_x and world_spawning_y. What are your thoughts?

The key idea we need to ensure is: "If the environment is constrained by dimensions, the spawning process must adhere to those limits. For example, if the boundaries are (-0.5, 0.5) for both x and y, the spawning process should respect these limits to ensure that entities remain within reach of one another.

In summary, the issue I'm explaining concerns how x_semidim and y_semidim are utilized from the kwargs.

[matteobettini]

Yes, makes sense, but if we do it this way the kwargs should be named world_spawning_x and world_spawning_y and default to 1

self.world_spawning_x = kwargs.pop("world_spawning_x", 1)
self.world_spawning_y = kwargs.pop("world_spawning_y", 1)

these would replace the kwargs we just introduced in the previous pr #133

the error is not needed anymore

[Giovannibriglia]

Yes, makes sense, but if we do it this way the kwargs should be named world_spawning_x and world_spawning_y and default to 1

self.world_spawning_x = kwargs.pop("world_spawning_x", 1)
self.world_spawning_y = kwargs.pop("world_spawning_y", 1)

these would replace the kwargs we just introduced in the previous pr #133

the error is not needed anymore

Understood, however my concerns regard the introduction of world_spawning_x and world_spawning_y in the kwargs. Will we apply this new logic across all suitable scenarios or only in the navigation scenario? If it's only for navigation, it might not be so elegant having these two only for one scenario, what do you think about?

Also, regarding setting 1 as default, I agree with you, it was my mistake.

[matteobettini]

I don't think we should consider families of related scenarios. Each scenario has its own kwargs and we don't guarantee any similarity across scenarios.

For example, discovery and navigation are different tasks, I don't think we should try to uniform them. The dense reward structure in navigation makes it suitable to unlimited bounds, while discovery needs bounding

[Giovannibriglia]

I don't think we should consider families of related scenarios. Each scenario has its own kwargs and we don't guarantee any similarity across scenarios.

For example, discovery and navigation are different tasks, I don't think we should try to uniform them. The dense reward structure in navigation makes it suitable to unlimited bounds, while discovery needs bounding

Perfect!! Now it sounds good :)

[Giovannibriglia]

sorry, I forgot them :/

Now, do I need to do anything else for this pull request?

[matteobettini]

Could we move this after the kwargs reading?

Also maybe a small comment after each of the 3 new kwargs would help

[Giovannibriglia]

Yes, it definitely helps. I tried to fix this in my latest commits :)

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.00%. Comparing base (ff58363) to head (631c2fa).

Files with missing lines	Patch %	Lines
vmas/scenarios/navigation.py	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
- Coverage   88.01%   88.00%   -0.02%     
==========================================
  Files          86       86              
  Lines        9845     9850       +5     
==========================================
+ Hits         8665     8668       +3     
- Misses       1180     1182       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[matteobettini]

Thanks a lot, we should be good!