-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Elevation profile plot #47990
Elevation profile plot #47990
Conversation
It looks like you are re-creating all the canvas/tools/mouseevent logic here. Wouldn't it have been possible to re-use the existing classes but stripping down the CRS notion or splitting them apart to have base common classes? |
767ab30
to
7b7bc08
Compare
@3nids and I discussed this offline, and the conclusion was that trying to keep a common base class between map canvas and the profile plot would result in more complex code/complex API in order to handle all the requirements of both canvases. |
Showcase time! Setting the profile curve line using a map tool: (Thanks to recent work by @3nids, it was dead-simple to allow the profile curve to take advantage of ALL the digitizing options, including snapping, curve digitizing, stream digitizing, cad dock, tracing, ....!) Peek.2022-03-31.16-27.mp4The line style for raster and mesh layers can be set in their corresponding layer properties - elevation tab: (Fun fact -- because these are rendered using standard QGIS line symbols, you can go really overboard with your profile tool appearance, by using geometry generators, paint effects, etc...! ) |
@nyalldawson A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
I personally think this PR is ready for merging now (at least as a first step!). Additional functionality can be added in follow up PRs. |
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.
I had a very very quick look, no objection!
@nyalldawson I see this PR is labeled for changelog and docs so can you update the first message, please? Thanks. |
4bf4c93
to
85805bb
Compare
Thanks, that's fixed now |
Done |
Unrelated test failure |
I believe these next comments are features but I just wanted to give a little feedback of the use of this new AWESOME feature ! 😃
All these comments do not change the excellent work done. |
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.
👏 👏 👏 Really great work!
Fantastic work! A few thoughts:
|
Thanks, it's definitely valued feedback!
I'm planning on addressing this in a followup (when the point cloud profiling is added). But yes, I agree that the performance on large raster layers in currently sub par.
That's supported in the new API, but I haven't yet exposed a means for the user to set the search tolerance yet. That'll be added in a follow up.
Agreed -- I'd like to see field/expression based support for both the height and the extrusion values. I'll see if I can fit this in, but no promises.
I've no plans for this at the moment, but I'll keep it in mind for future work.
That's planned as a follow up PR.
Right now the tool is using the assumption that the user wants to see all their project elevation enabled layers on the same plot. I also think we'll need to revise this in future, but I'm not sure what the best way to expose this will be. I'm leaning toward showing a filtered, checkable layer list on the left of the plot showing only elevation enabled layers, allowing users to check and rearrange the plot layers. But I'm also concerned about the loss of plot area this would cost. Ideas welcome 😄 |
…tting plot ranges
2fa215e
to
6a4458f
Compare
I've added a dedicated tool for x-axis zoom only now, and put the action for it before the standard zoom.
I'll try to find time to move the rendering to a background thread in a follow up
I've just added support for selecting the band in the layer- elevation properties tab.
Thanks, fixed |
@nyalldawson |
Done in #48177 |
Done in #48179 |
Done in #48248 |
Hi nyall, are you planning to present an option to set a "same scale axis" like in ProfileTool ? We funded this feature in the plugin because the different x/y scales for the same unit of measure often were a source of misreading for our users, the default scale behaviour being surprising for those coming from CAD tools (road profiles, building cross sections, river bed, etc.). |
It's not in the scope of the current project, but would definitely be possible! |
This PR implements a native elevation profile tool.
Currently supports:
Some demonstrations:
Setting the profile curve line using a map tool:
Peek.2022-03-31.16-27.mp4
The profile chart style for layers can be set in their corresponding layer properties - elevation tab:
(Fun fact -- because these are rendered using standard QGIS line symbols, you can go really overboard with your profile tool appearance, by using geometry generators, paint effects, etc...!
)