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

Map overview #149

Merged
merged 19 commits into from Dec 14, 2018
Merged

Map overview #149

merged 19 commits into from Dec 14, 2018

Conversation

slimonslimon
Copy link
Contributor

This Improvement of Browse page add small pre-generated image of map with GPS track.
It can improve orientation in logs on browse page.
Overview image is generated when user visits plot_app page for first time and the image is saved in data/img direcotry.

@bkueng
Copy link
Member

bkueng commented Dec 7, 2018

Awesome! Really neat.
A few notes:

@slimonslimon
Copy link
Contributor Author

Thank you for your sugesstions!

But I have one question about general concept of preview image: I planned to have preview image generated for all saved .ulog files - it is generated when someone visit the plot page. So browse page should be loaded as fast as possible independet to generation time. (I will write script which generate preview images for all existing logs which can be run when server idle)
Do you have better idea?

Other suggestions are clear, i will implement them.

@slimonslimon
Copy link
Contributor Author

the map is now generatd faster.

Other problem is that Im not able to get continues integration tests to work, because im not familiar with pipenv and I have only python 3.5

@bkueng
Copy link
Member

bkueng commented Dec 13, 2018

I agree we should not generate them during a browse page request, it would be too slow. However since it's not immediately required after upload when loading the plot page, I also don't want to add more delay to that. This is why I suggested to do it on idle (something like this), and/or asynchronous (e.g. in a separate process).
Since the browse page only contains public logs, we can limit the generation to these logs.

Other problem is that Im not able to get continues integration tests to work, because im not familiar with pipenv and I have only python 3.5

No worries, I can look into that.

@slimonslimon
Copy link
Contributor Author

I moved generation to upload page (only when public) and i tried to run it in sepate task, but im, not sure if my code is corect - im not familiar with tornado async api (and i dont know much about async task programing)

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

That is good. It's now run after the upload request finished and before the plot page is loaded - which is ok, as it's below 1 second now.

One thing I noticed is that the 'Not rendered / No GPS' label is zoomed as well. Is there a way to disable that?

@@ -0,0 +1,24 @@

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this script executable? -> chmod +x generate_overview_img.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course

@slimonslimon
Copy link
Contributor Author

Ok, It is solved by generating other html, when preview image doesnt exist.

I did not found any solution with CSS - for aligement I need display: block - but it is applied to alt text too.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

I think we have it now, thanks for your efforts.
For CI there are some errors:

plot_app/overview_generator.py:13:0: C0103: Argument name "z" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern (invalid-name)
plot_app/overview_generator.py:19:4: C0103: Variable name "sx" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern (invalid-name)
plot_app/overview_generator.py:19:8: C0103: Variable name "sy" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern (invalid-name)
plot_app/overview_generator.py:52:8: C0103: Variable name "z" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern (invalid-name)
plot_app/overview_generator.py:54:12: C0103: Variable name "z" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern (invalid-name)

Can you either rename them or add them as exceptions in https://github.com/PX4/flight_review/blob/master/pylintrc#L102? Either way is fine with me.

Pipfile Outdated Show resolved Hide resolved
@slimonslimon
Copy link
Contributor Author

Im stil not able to get Pipfile compatible with requirementes.txt

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

CI passes, so it's good to go

@bkueng bkueng merged commit cb0da8f into PX4:master Dec 14, 2018
@bkueng
Copy link
Member

bkueng commented Dec 14, 2018

It's working: https://logs.px4.io/browse.
But I had to fix another thing when running on the server: 0b5a8f6

@slimonslimon
Copy link
Contributor Author

Thank you for your help.

@slimonslimon slimonslimon deleted the map_overview branch January 29, 2019 10:54
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.

None yet

2 participants