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

POC fix for legacy lifts #126

Merged
merged 3 commits into from
Jul 26, 2023
Merged

POC fix for legacy lifts #126

merged 3 commits into from
Jul 26, 2023

Conversation

luca-della-vedova
Copy link
Member

Bug fix

Fixed bug

When importing projects containing lifts with rectangular shapes, things can go quite wrong, from swapped widths and depths to failure in importing the project.
This PR is little more than a bug report, it's a "hacky fix that seems to work", there might be a better fix but I'm not familiar with how lifts are stored and parsed in the site editor.

Fix applied

I just swapped widths and depths in the legacy lift importing code path, not sure how the coordinate system is supposed to be but this seems to fix all the scenarios I could find.

Test it!

  • Create a world with a rectangular lift, for example change this line to a large number like 10.0
  • Open the map in traffic editor to see what it looks like.
  • Try to open the map in site editor main branch, import should fail: Failed converting to site InvalidLiftCabinDoorPlacement { lift: "Lift1", door: "lift1_door" }
  • Try the PR, import should be successful and the lift should match its traffic editor rendering

Signed-off-by: Luca Della Vedova <[email protected]>
Copy link
Member

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

While these changes work for me, reading the code is really confusing. Consider the following options:

  • Add a comment as to why we are flipping the width and the depth before
  • Flip the usage of d and w where they are used in function calls.

Copy link
Member

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

While these changes work for me, reading the code is really confusing. Consider the following options:

  • Add a comment as to why we are flipping the width and the depth before
  • Flip the usage of d and w where they are used in function calls.

@luca-della-vedova
Copy link
Member Author

Yea I have no hope of getting this merged, it's really meant to be just a bit more of a bug report, a "bug report with a way to work around it that hopefully sheds some light into what a proper fix might be".
Will change it to draft to reflect this

@luca-della-vedova luca-della-vedova marked this pull request as draft May 17, 2023 09:48
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey mxgrey marked this pull request as ready for review July 26, 2023 04:47
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Thanks for noticing and reporting this. You're very correct, the width and depth should be flipped when we do the conversion. That's actually the correct fix, and the reason is because of a change in coordinate axis conventions.

The legacy format used a simple y-forward, x-sideways convention for depth and width, probably because when the lift has zero rotation then the x-axis "feels" like the dimension that should be called "width". However for the site editor I decide to switch to the robotics convention of the local x axis being forward/backward and the local y axis being side-to-side, because I believe that will be more consistent with conventions that we'll use elsewhere, and I believe it will make more sense for lifts that have some rotation.

I've simply added a note about this into the PR because it's admittedly confusing. No other action should be needed to fix the problem.

@mxgrey mxgrey dismissed arjo129’s stale review July 26, 2023 04:52

The suggestion of adding a comment has been fulfilled.

@mxgrey mxgrey merged commit 54a5b45 into main Jul 26, 2023
5 checks passed
@mxgrey mxgrey deleted the throwaway/lift_size_import branch July 26, 2023 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants