-
Notifications
You must be signed in to change notification settings - Fork 75
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
[Feature] Rendering boundaries in dimension-limited environments #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! I left comments
Could you test this (even just by yourself visually) with many different values and combinations of:
- viewer_zoom
- viewer_size
- render_origin
These should not affect the boundaries but better to make sure
@@ -740,6 +740,51 @@ def render( | |||
pos[Y] + cam_range[Y], | |||
) | |||
|
|||
# Include boundaries in the rendering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a subfunction for this
from vmas.simulator.utils import Color | ||
|
||
# Check if the world dimensions are defined | ||
if self.world.x_semidim is not None and self.world.y_semidim is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we handle also when only one is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't see any issues with that approach.
However, my main concern lies in how to structure the if-condition for enabling boundary visualization.
- Currently, boundary visualization is triggered by checking whether the
x_semidim
andy_semidim
variables are defined:if self.world.x_semidim is not None and self.world.y_semidim is not None: view bounds...
- Should we standardize this by ensuring that every scenario includes
x_semidim
andy_semidim
to ensure consistent boundary visualization? - Are there any alternative methods or conditions you would suggest for enabling boundary visualization more effectively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if x_semidim is enabled we visualize x. If y semidin is enabled we visualize y
# Check if the world dimensions are defined | ||
if self.world.x_semidim is not None and self.world.y_semidim is not None: | ||
# Get the radius of the agents | ||
radius = self.agents[0].shape.radius |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite brittle as agents may have different sizes. How should we handle?
- do not add radiuses
- add the max radius
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which scenario are you referring to? Just to have a concrete example, thanks.
This is purely a visualization issue, not something more, since I believe that (center + half_radius) should never exceed the boundary limits. Right?
The safest approach would be to use the max_radius (where can I get it?), but this could cause some visualization issues, such as smaller agents not reaching the visualized boundary.
Suggestions? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly,
with 2 you have smaller agents not reaching the boundary, with 1 the boundary is always for the center of agents
this is partly what concerns me about this PR: it is impossible to have a boundary that fits all agents
|
||
# Define the corner points of the boundary | ||
corners = [ | ||
(origin_x - x_semi, origin_y + y_semi), # top_left |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? the origin of the rendering is where the camera is, but the boundaries are always from the physical origin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've conducted some tests, and it turns out that the choice of
origin_x, origin_y = (0, 0)
would be the correct approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meaning we can remove the origin from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, anyway we could left them as variables initialized to (0, 0), which might be useful for future enhancements. Do you agree?
c807dbd
to
89882a6
Compare
I reforked the repo, therefore the pull request has been closed :/. |
Description:
Changes Made: