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/webapi-mapfeatures #826

Merged
merged 43 commits into from
Mar 8, 2022

Conversation

KOKAProduktion
Copy link
Contributor

@KOKAProduktion KOKAProduktion commented Dec 16, 2021

Hi @albar965, hi @u-an-i,

this PR includes and therefore replaces #788 .

It provides

This is just to abandon the #788 branch on my side. I don't see the need in maintaining it separately anymore. No review nor merge required at this point.

Cheers!

- Add new route to API contract with default values (must be removed after development)
- register new MapActionsController at ActionsControllerIndex
- PoC example only
- Proove rendering retrievable via api
- Replace default values by "example" values from api contract
- Compatible with initial https://github.com/KOKAProduktion/littlenavmap-openlayers/tree/feature/lnm-webapi
- Make copyright note painting setable via MapPaintWidget::setPaintCopyright
- Enable small distance Rect requests (closer zoom)
- albar965#784 (comment)
- Implement airports restrieval by rect
- Implement /map/features route
- Fetches feature results from in-app map cache (wrong)
- Extend info builders
- Complement API contract

Core changes:
- Expose MapPaintWidgets MapPaintLayer via getMapPaintLayer()
- Extend MapQuery by getAirportsByRect(...)
- Apply albar965#790 (comment)
- Perform dummy image action and extract results
- Expose MapPaintLayer::updateLayers() to allow mapLayer initialisation
- Add NDB, VOR, and Marker data
- Extend MapQuery by atools Rect query methods
- Extend info builder
- NDB,VOR,Marker results appear not to match airport retrieval rect
- setShowMapFeatures -> setShowMapObject
- setShowMapFeaturesDisplay -> setShowMapObjectDisplay
- setShowMapFeatures -> setShowMapObject
- setShowMapFeaturesDisplay -> setShowMapObjectDisplay

(cherry picked from commit b3330c5)
Comment on lines +205 to +206
// Fill result object
mappixmap.pixmap = mapPaintWidget->getPixmap(width, height);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#848 Hi @albar965, I've started using MapPaintWidget::getPixmap() here but as far as i can see this hasn't been merged yet. Still won't exclude that I may have introduced the issue on the #772 and #776 PR's...

Should I check or investigate further?

Cheers

Copy link
Owner

Choose a reason for hiding this comment

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

Would the merge the problem? Or reverting a change?
And there is still the issue with getIls().

I can simply release it the next days. It's an alpha/development version. People can expect issues. 🙂

Alex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, i don't think merging would solve the issue. At least I haven't run into it yet so i didn't fix anything in this PR. So maybe we shouldn't merge before we solve it, trying to avoid adding issues :)

Not retrieving ILS yet on this PR, I'm only that far for now:

        const QList<map::MapAirport> airports;
        const QList<map::MapNdb> ndbs;
        const QList<map::MapVor> vors;
        const QList<map::MapMarker> markers;
        const QList<map::MapWaypoint> waypoints;

Cheers

Copy link
Owner

Choose a reason for hiding this comment

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

Nah, i don't think merging would solve the issue. At least I haven't run into it yet so i didn't fix anything in this PR. So maybe we shouldn't merge before we solve it, trying to avoid adding issues :)

Not retrieving ILS yet on this PR, I'm only that far for now:

Happens during paint in the marble widget.

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'll investigate

Copy link
Owner

Choose a reason for hiding this comment

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

Ok. Take your time.

@u-an-i
Copy link
Contributor

u-an-i commented Feb 22, 2022

Hi @KOKAProduktion ,

I looked through, partly skimmed, https://github.com/albar965/littlenavmap/pull/826/files :

looks good** !

** I don't remember the context of everything and didn't "verify" every logic by thinking them through but nothing triggered any "knowledge" of mine about an improvement to / how to improve your additions and changes.

To summarise your code and the api contract you currently added returning features such as airport data for a given "LatLonRect" (expression not looked up as being used in the code technically), usable for example to add layers of such data in the future upon a OL tile.

I just saw
image
passed me (or I forgot about it) :(

I'll see whether I can report about that too.

@KOKAProduktion
Copy link
Contributor Author

Hi @u-an-i,

thanks for your review! It's still a draft since it's missing some features. I'm on it.

While developing I've realised that creating layers with interact-able objects is not required in the first step as long as they are still painted on the map image. That's why I diverted to just query objects close to the users click. This also reduces the load on the browser.

I'm aiming to finish a replacement for the current mapping solution in the web UI soon, providing all the currently available features. I'm just missing the wind indicator/speed at the top atm...

In my implementation The OL map object is available to the web UI's JS and fires subscribe-able events passing requested API data to any consumer. (Aircraft performance, click result, etc). This way we'll be able to reuse the API call results the map requires for its functioning in the web UI JS. Also controlling the map (e.g. by your auto-zoom plugin) is possible through the exposed object.

In a second step or when we decide to move painting from LNM/Marble to OL querying for all visible objects in the viewport becomes required.

Please give me some more time (2 weeks maybe) to finish this PR for completeness... I don't want to waste your time reviewing unfinished stuff :)

Thanks again and cheers!

@u-an-i
Copy link
Contributor

u-an-i commented Feb 22, 2022

Thank you very much for your detailed reply!

I understand and agree.

I very surely give you more time, as much as you need and even don't need :)

@KOKAProduktion

@KOKAProduktion
Copy link
Contributor Author

Hi @albar965, hi @u-an-i,

I think I'm ready to integrate the OL map into the default web UI. As far as I remember, both the existing single image and the OL solutions shall be toggle-able by the user, right? I'd start with that tomorrow.

Thanks and cheers

@u-an-i
Copy link
Contributor

u-an-i commented Mar 7, 2022

@KOKAProduktion

I would like the functionality of the current web ui (button bar, themes, plugins, framed content eg map) and the legacy ui be present in an ol version.

For this you can either replace the map.html called in the new web ui by an map_ol_html or create a v3 an keep the "new web ui" and the legavy web ui as 2 legacy uis.

If you go with 2 lgecys uis you are free to do whatever you want - as everyone always is supposed to be - but if you go with replacing the map in the new ui please read the plugin doc and the "planning" doc or how i called it which covers the conformance any update should have.

I'm looking intriguedly forward!

@KOKAProduktion
Copy link
Contributor Author

I would like the functionality of the current web ui (button bar, themes, plugins, framed content eg map) and the legacy ui be present in an ol version.

Yep. I'm aiming for that too. Ideally I'd like to let the user toggle the mapping solution only. I'm just afraid that this may increase maintenance on the web ui side (keeping up functionality on both solutions). But I don't assume that everything will be perfectly satisfying on the OL solution up front, so I guess we have to go this way anyway at least in the beginning.

please read the plugin doc and the "planning" doc

Could you please point me to these docs? Would you like me to update your plugins or would you like to take care of that yourself?

Thx and cheers!

@u-an-i
Copy link
Contributor

u-an-i commented Mar 7, 2022

@KOKAProduktion
Copy link
Contributor Author

Alright guys!

Finally here it is :) This is the OL map solution inside the LNM web UI, implemented following @u-an-i 's plugin pattern. There are some considerations regarding dynamic objects display and the clickability is already available but not yet connected to anything.

Just reflecting the current feature set of the single image mapping solution, from my point of view.

Also the plugin active state seems not to be persisted over sessions but since the autozoom plugin doesn't persist too, i guess that's still in progress by @u-an-i.

I haven't adapted the autozoom plugin to the OL solution, but the littlenavmap object holding the ol map is now exposed inside the content iframe, so it can be accessed via the OL API there.

I'd love to contribute much more and faster, but current circumstances force me to take care of other important things. I'm afraid I won't be able to do much the upcoming weeks or even months... Sorry for that! But I'll rejoin as soon as i can and I'm still here for fixes, issues and discussions. Meanwhile I'll try to extend the documentation on this.

I hope you like it and I'm very curious about your impressions and remarks!
Have a nice evening and cheers!
Kosma

@KOKAProduktion KOKAProduktion marked this pull request as ready for review March 8, 2022 16:22
@albar965
Copy link
Owner

albar965 commented Mar 8, 2022

Can I merge? I reviewed it already some time ago.

@KOKAProduktion
Copy link
Contributor Author

Yes you can! I think it's stable and not obstructing other features.

@u-an-i
Copy link
Contributor

u-an-i commented Mar 8, 2022

woohoo! :)

Also the plugin active state seems not to be persisted over sessions but since the autozoom plugin doesn't persist too, i guess that's still in progress

this was a decision to not do so but I'm open to change this!

take care of other important things

perfectly good!

I'm very curious about your impressions

I'm so too! :) And I'm so for another current project of mine. I guess i look a the web code quickly right away but carefully only later!

@u-an-i
Copy link
Contributor

u-an-i commented Mar 8, 2022

quickly looked:

oh, this is concise while spaciously presented: nice!

@albar965 albar965 merged commit 08f4e53 into albar965:master Mar 8, 2022
@albar965
Copy link
Owner

albar965 commented Mar 8, 2022

Thanks a lot! 🎆 🏅
Alex

@KOKAProduktion
Copy link
Contributor Author

Thank YOU guys! Especially for all the support on the way! It was a lot of fun and I hope I'll be able to return soon, as there's so much more to do :)

Cheers!

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