-
Notifications
You must be signed in to change notification settings - Fork 399
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
Python viewer #1519
Python viewer #1519
Conversation
Very cool. I was able to test and reproduce on a mac with building from source. However, it only works with
|
@JuanTheEngineer Pulling from master should resolve the CI CA issues. |
@JuanTheEngineer You also need to run pre-commit on this PR for proper formatting + linting. |
Yes, this is related to the issue @Skylion007 mentioned. For now @JuanTheEngineer thanks for the thorough instructions in the testing section. Maybe add a line that building with |
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.
Once #1529 is merged, these two changes should allow you to resize the window. However, viewer.cpp's viewportEvent() has a lot of code to resize all existing framebuffers and I'm not sure how/if these are exposed to Python. So it's possibly not even implementable at this point.
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.
Instead rebasing, you may just want to pull from main next time as it would also make reviewing this PR a bit easier.
ae877b7
to
8134041
Compare
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.
Looks good, almost ready. A couple of notes from our chat.
action_queue: List[str] = [act[k] for k, v in press.items() if v] | ||
|
||
for _ in range(int(repetitions)): | ||
[agent.act(x) for x in action_queue] |
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.
Love this reduction. 👍
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.
Looks good. Let's merge it. 🥳
Motivation and Context
All features can be seen in help text.
Previous PRs: 727 & 1056
How Has This Been Tested
In order to run the viewer, you must use the commands below.
Types of changes
Checklist