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

[Feature] Boundary visualization for limited-size environments #142

Merged
merged 19 commits into from
Sep 20, 2024

Conversation

Giovannibriglia
Copy link
Contributor

This tries to fix #134

@Giovannibriglia
Copy link
Contributor Author

Let me know what do you think about it. I am not so sure regarding ''infinite_value = np.infty"

@@ -77,6 +77,7 @@ def __init__(
self.headless = None
self.visible_display = None
self.text_lines = None
self.visualize_semidims = kwargs.pop("visualize_semidims", True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we can put in the Scneario class like the other rendering options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok :)

# Check boundary limits
if self.world.x_semidim is not None or self.world.y_semidim is not None:
# Get the origin coordinates
origin_x, origin_y = (0, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the origin as this will always be the physical origin in 0,0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok :)

# Get the origin coordinates
origin_x, origin_y = (0, 0)

infinite_value = np.infty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test what np.infty plots? does it work? Maybe there is a way to plot an infinite long line in pyglet, we should check, otherwise we can choose a very high value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I haven't been able to test it yet, which is why I was hoping to get your feedback. :) In other instances, it seems to work fine.

Using np.infty doesn't seem like the best approach. I couldn't find any guidance on how to implement an infinite line or one that progressively extends. For now, the simplest option seems to be setting infinite_value to a very large number (perhaps 10^9?). What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that might be best

To test it just run a scenario of your choice and play with the args:

  • one boundary only, both, none
  • also play with the rendering settings in the Scenario base class to check that they do not affect the rendering of the boundaries

Copy link
Contributor Author

@Giovannibriglia Giovannibriglia Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have just tried this in navigation, it should works fine.

Let me resume what I have done in order to assess if it is ok:

        if self.enforce_bounds:
            self.x_semidim = None # self.world_spawning_x
            self.y_semidim = self.world_spawning_y
        else:
            self.x_semidim = None
            self.y_semidim = None
  • I have run the simulation and this is the rendering result:
    image

If I try with y_semidim, the result is analogous.
It should be okay, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks very good

also try changing these settings from the make_world in navigation to check they interact well with the feature

self.viewer_size = INITIAL_VIEWER_SIZE
"""The size of the rendering viewer window. This can be changed in the :class:`~make_world` function. """
self.viewer_zoom = VIEWER_DEFAULT_ZOOM
"""The zoom of the rendering camera (a lower value means more zoom). This can be changed in the :class:`~make_world` function. """
self.render_origin = (0.0, 0.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops, yeah it should works well, I didn't say you previously :)

I have set:

  • self.viewer_size = (640, 540)
  • self.viewer_zoom = 0.6
  • self.render_origin = (-2, 1)

Here the result:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! ok push and i ll review

@matteobettini matteobettini added the enhancement New feature or request label Sep 18, 2024
@matteobettini matteobettini changed the title add boundary visualization for limited-size environments [Feature] Boundary visualization for limited-size environments Sep 18, 2024
@matteobettini
Copy link
Member

matteobettini commented Sep 18, 2024

Ok, now, there are some scenarios that already plot their boundaries

We need to set self.visualize_semidims=False in these scenarios in order to keep their old buondar plottiing, they are:

  • passage
  • joint_passage_size
  • joint_passage
  • ball_passage
  • sampling
  • football
  • read_traffic
  • simple_tag
  • balance

)

# define edges
boundary_edges = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of only one boundary set we need just 2 lines not 4

Copy link
Contributor Author

@Giovannibriglia Giovannibriglia Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of only one boundary set we need just 2 lines not 4

Sorry, are you sure?

Eventhough are just two lines, in my opinion, we should specify 4 points. Anyway, I agree that the previous implementation drew a square/rectangle.

I think the point regards the for-loop for drawing lines, for this reason I suggest this new implementation. But I'm not understanding why it draws nothing in the case of just "two parallel lines".

if x_semi is not None and y_semi is not None: # drawing square/rectangle

    boundary_edges = [
        (-x_semi, y_semi),  # top_left
        (x_semi, y_semi),  # top_right
        (x_semi, -y_semi),  # bottom_right
        (-x_semi, -y_semi)  # bottom_left
    ]

    # Create lines to form the boundary by connecting each corner to the next
    for i in range(len(boundary_edges)):
        start = boundary_edges[i]  # Current corner point
        end = boundary_edges[
            (i + 1) % len(boundary_edges)]  # Next corner point, wrap around to the first point
        print(start, end)
        line = Line(start, end, width=0.7)  # Create a line between two corners
        line.set_color(*color)  # Set the line color
        self.viewer.add_onetime(line)  # Add the line to the viewer for rendering

else: # drawing two parallel lines

    # if only x_semi is provided, draw horizontal lines
    if x_semi is not None:
        boundary_edges = [
            (-x_semi, infinite_value),  # upper_left
            (x_semi, infinite_value),  # upper_right
            (-x_semi, -infinite_value),  # lower_left
            (x_semi, -infinite_value)  # lower_right
        ]

    # if only y_semi is provided, draw vertical lines
    elif y_semi is not None:
        boundary_edges = [
            (infinite_value, y_semi),  # right_top
            (infinite_value, -y_semi),  # right_bottom
            (-infinite_value, y_semi),  # left_top
            (-infinite_value, -y_semi)  # left_bottom
        ]

    else:
        boundary_edges = []

    # create the two parallel lines by connecting the points
    for i in range(0, len(boundary_edges), 2):
        start = boundary_edges[i]
        end = boundary_edges[i + 1]
        print(start, end)
        line = Line(start, end, width=0.7)
        line.set_color(*color)
        self.viewer.add_onetime(line)

What do you think?

Copy link
Contributor Author

@Giovannibriglia Giovannibriglia Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong in labelling boundary_edges, the correct name should be boundary_points or something like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes what you did is what i was thinking, still 4 points, but only draw 2 lines when only one semidim is available.

I do think we can write it a little more compactly tho:

  • define the 4 points like before
  • write only once the line creation loop making it work for 2 or 4 lines

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is too difficult also as is it works, but more compact would look better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what we want to avoid is drawing 4 lines when only one semidim is available

@Giovannibriglia
Copy link
Contributor Author

Ok, now, there are some scenarios that already plot their boundaries

We need to set self.visualize_semidims=False in these scenarios in order to keep their old buondar plottiing, they are:

  • passage
  • joint_passage_size
  • joint_passage
  • ball_passage
  • sampling
  • football
  • read_traffic
  • simple_tag
  • balance

Yes, you're correct. Is it just a matter of inserting self.visualize_semidims = False into these scripts?

@matteobettini
Copy link
Member

Yes, you're correct. Is it just a matter of inserting self.visualize_semidims = False into these scripts?

Exactly

@Giovannibriglia
Copy link
Contributor Author

Giovannibriglia commented Sep 20, 2024

Now it should be fine :)

  • Is setting infinite_value = 100 sufficient?
  • Does the "impossible case" in the if statement need to be handled? And if so, have I implemented it correctly?

@Giovannibriglia
Copy link
Contributor Author

I attempted to set self.visualize_semidims = False consistently before world creation. Does this seem okay?

Also, I couldn't locate the simple_tag scenario :/

Copy link
Member

@matteobettini matteobettini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nits

vmas/simulator/environment/environment.py Outdated Show resolved Hide resolved
vmas/simulator/environment/environment.py Outdated Show resolved Hide resolved
Comment on lines 809 to 814
boundary_points = [
(-x_semi, y_semi),
(x_semi, y_semi),
(x_semi, -y_semi),
(-x_semi, -y_semi),
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks the same as the first part of the if

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just fixed

@matteobettini
Copy link
Member

I attempted to set self.visualize_semidims = False consistently before world creation. Does this seem okay?

Also, I couldn't locate the simple_tag scenario :/

it is in scenarios/mpe

@matteobettini
Copy link
Member

Now it should be fine :)

  • Is setting infinite_value = 100 sufficient?
  • Does the "impossible case" in the if statement need to be handled? And if so, have I implemented it correctly?

100 seems decent, higher values do not render for me, we can always change it later

color = Color.GRAY.value

# Define boundary points based on whether world semidims are provided
if (self.world.x_semidim and self.world.y_semidim) or self.world.x_semidim:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (self.world.x_semidim and self.world.y_semidim) or self.world.x_semidim:
if (self.world.x_semidim and self.world.y_semidim) or self.world.y_semidim is not None:
  • i think it was y not x
  • always use is not None as it might trigger or not when it is 0 or not (imagine the nightmare)

Copy link
Contributor Author

@Giovannibriglia Giovannibriglia Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for this; should we add "is not None" also in the other parts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i think all of this got a bit messed up, i'll let you test it and then i ll do the last test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the caos :(

I wanted to ask if I should include 'is not None' in other if-statements like:

  • x_semi = self.world.x_semidim if self.world.x_semidim else infinite_value --> x_semi = self.world.x_semidim if self.world.x_semidim is not None else infinite_value (and similarly for y_semi)
  • 1 if (self.world.x_semidim and self.world.y_semidim) else 2 --> 1 if (self.world.x_semidim is not None and self.world.y_semidim is not None) else 2

I've tested all the scenarios with and without 'is not None,' and they worked fine.

But just to be on the safe side, as you suggested, I'll add 'is not None' wherever possible. Does that sound good?

@matteobettini
Copy link
Member

I believe we are there Gio!

@matteobettini matteobettini merged commit 132d97b into proroklab:main Sep 20, 2024
4 checks passed
@Giovannibriglia
Copy link
Contributor Author

Yeeeee, great!!!

Thank you for your patience :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualizing boundaries in dimension-limited environments
2 participants