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

Generating profiles using profile rest API #76

Merged
merged 12 commits into from
May 22, 2024

Conversation

gacarrillor
Copy link
Contributor

@gacarrillor gacarrillor commented Apr 26, 2024

Pending:

  • Line/polygon symbology.
  • Profile visible in Layout.
  • Export 3d (x, y, z).
  • Export 2d (distance, z).
  • Export distance/elevation table.
  • Snap to profile results.
  • Bug in EPSG:3857: Displaced profile (wontfix for now).
    • Note there is a QGIS bug here and a related bug here).
  • Layer tree node (only in a follow-up).
  • Identify: Does not apply (no features on the main canvas to get attributes from)
vokoscreenNG-2024-05-07_14-55-55.mp4

@gacarrillor gacarrillor marked this pull request as draft April 26, 2024 15:04
Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

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

niiiiiiiiice! looking forward to seeing a demo!

swiss_locator/core/profiles/profile_generator.py Outdated Show resolved Hide resolved
swiss_locator/core/profiles/profile_generator.py Outdated Show resolved Hide resolved
swiss_locator/core/profiles/profile_generator.py Outdated Show resolved Hide resolved
swiss_locator/swiss_locator_plugin.py Outdated Show resolved Hide resolved
@gacarrillor
Copy link
Contributor Author

niiiiiiiiice! looking forward to seeing a demo!

Just posted a demo v0.1 in the PR description (several things still missing).

@3nids
Copy link
Member

3nids commented Apr 30, 2024

I don't the the demo.
Anyway, let me know if it's ready on your end and we move forward!

@gacarrillor
Copy link
Contributor Author

I don't the the demo. Anyway, let me know if it's ready on your end and we move forward!

Sorry, don't know what happened with the demo. Just reposted it in the description.

I'll update the description with missing stuff so that you get an idea on its status.

@3nids
Copy link
Member

3nids commented May 1, 2024

Cool, good job!!!

@gacarrillor gacarrillor marked this pull request as ready for review May 3, 2024 14:36
@gacarrillor
Copy link
Contributor Author

gacarrillor commented May 7, 2024

@3nids I think we've reached the goals for this PR. I'd appreciate if you can give it a test and check if the code looks good for you.

I'll add an up-to-date screencast to the PR description in a while.

Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

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

Looking awesome!

snapped_z = (dy / dx) * (point.distance() - prev_distance) + prev_elevation

if abs(point.elevation() - snapped_z) > context.maximumSurfaceElevationDelta:
return QgsProfileSnapResult()
Copy link
Member

Choose a reason for hiding this comment

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

not exactly sure, is it ok to exit the loop here, no chance to get a good result at a next iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I borrowed the code from the current QGIS profile implementation, but giving it a deeper read, I see that we can only reach that if clause (line 103) if we're already in the good profile segment (i.e., see line 98's condition), so if we don't return immediately, we'll be accepting the previous if condition (line 98) evaluation for all the other segments where the initial point distance value is not included. All other segments would be rejected as the condition prev_distance <= point.distance() <= k won't be met. So, returning in line 104 is allowing us to avoid line 98's if condition being evaluated repeatedly without never really entering it again.

try:
from swiss_locator.core.profiles.profile_generator import SwissProfileSource
except ImportError:
# Should fail only for QGIS < 3.26, where profiles weren't available
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Should fail only for QGIS < 3.26, where profiles weren't available
# Should fail only for QGIS <= 3.36, where profiles weren't available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it's 3.26, since that's the QGIS version where Profile functionality landed.
However, I'm checking against 3.37, because before 3.37 we cannot really register any QgsAbstractProfileSource subclass.

@gacarrillor
Copy link
Contributor Author

Rebasing to solve conflicts with the recently merged VectorTiles branch.

@3nids 3nids merged commit 0e64bc7 into opengisch:master May 22, 2024
2 checks passed
@3nids
Copy link
Member

3nids commented May 22, 2024

Awesome!

@nirvn
Copy link
Member

nirvn commented May 22, 2024

Just passing by to also drop a "awesome!" here :)

@gacarrillor gacarrillor deleted the profiles branch May 22, 2024 13:23
@gacarrillor
Copy link
Contributor Author

Thank you!
Just be aware that there is still something we should solve. The ESPG:3857 gives us displaced profile graphs (See #86)

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