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

Issue 557 updates to plot 3d fr py #558

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tm132
Copy link
Contributor

@tm132 tm132 commented Oct 1, 2022

Closes: #557

Updated scripts/plot_3d_fr.py - Quality of Life updates:

  • fixed an index error bug removed this as there are bigger issues at play that need to be looked into more in-depth
  • added -i option for interactive mode. The gui will no longer be displayed by default. If you want to run the gui, use the -i (interactive) flag from now on.
  • made the default output directory the same directory that frames.bin was found in.
  • added -o (output directory) option which will allow the user to specify an alternative output directory for writing out the plots.

Updated proto_utils.py

  • make the error message in read_frames() more detailed/descriptive

These updates were made for the purpose of allowing this plotting script to be used in an automated process (switching it from interactive-by-default to automatic-by-default (no gui) as well as adding an optional command line option to specify the output directory of your choice).

…added -i option to denote interactive mode where the gui will be launched, requiring user interaction. by default, this script will simply output the plot (2d or 3d) automatically and write it to the directory where frames.bin was found (by default). added an option -o to specify an alternate output directory.
… explicit in the output print about what went wrong.
@tm132
Copy link
Contributor Author

tm132 commented Nov 17, 2022

While I think there are some good QoL updates here (the updates to the invocation options), there is something fundamentally wrong with how this seems to attempt to plot data that likely has at least something to do with the index out of bounds error line. It is using contact.id.id-1 instead of just contact.id.id, making the assumption that entity_id will never be lower than 1, and then shifting all entity ids down by 1 and using them as the index of an array. Some simulation outputs I was testing with were causing an index out of bounds error because of this assumption.

The fix for this index out of bounds error in the PR is a band-aid, not a real robust solution. It should probably be replaced with a map(), or have a more in-depth reimplementation, or maybe even just re-evaluated for overall usefulness.

Because this was producing plots that weren't right, instead of fixing this utility more completely, I instead used it as an example of how to interact with scrimmage's python package for parsing frames.bin, and incorporated a new frames.bin translator into my own project's post processor tool, which is producing correct-looking plots from the frames.bin data.

I'm on the fence about what to do about this one:

  • revert the fix conidx = min(con.id.id - 1, 0) back to the original conidx = con.id.id - 1 and then merge in the PR to get all the QoL updates
  • just decline this PR and move on

…d not address the fundamental issue of why the entity id was being shifted down by one and used as an array index. this needs to be reevaluated for a more in-depth reimplementation of how the data is stored in order to be processed (use a map instead of an array?) or if this script is really useful enough to maintain in the first place
@tm132
Copy link
Contributor Author

tm132 commented Nov 17, 2022

I decided to go ahead and remove the one-line fix for the index out of bounds error I was getting. The only thing remaining in the PR are the QoL updates. This PR can be merged if the QoL updates are desireable.

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.

Update plot_3d_fr.py
1 participant