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

Merge remote and local QgsEptPointCloudIndex classes #59279

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

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented Oct 30, 2024

Description

This PR implements part of qgis/QGIS-Enhancement-Proposals#290: It removes QgsRemoteEptPointCloudIndex, putting its code into QgsEptPointCloudIndex, which can now be used for both remote and local data.

It also slightly refactors QgsPointCloudIndex, changing nodeChildren() so it uses virtual methods and we don't have to override it, and removing a duplicate nodePointCount() method (confusingly, two variants, const and non-const, existed in different parts of the file).

@github-actions github-actions bot added this to the 3.42.0 milestone Oct 30, 2024
Copy link

github-actions bot commented Oct 30, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit aad96a7)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit aad96a7)

@qgis qgis deleted a comment from github-actions bot Nov 1, 2024
@qgis qgis deleted a comment from github-actions bot Nov 1, 2024
@dvdkon dvdkon force-pushed the ept-remote-merge branch 2 times, most recently from c2101b3 to 37bbc44 Compare November 12, 2024 08:18
@dvdkon dvdkon marked this pull request as ready for review November 12, 2024 20:59
Copy link
Member

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

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

This looks great!

@@ -26,21 +26,31 @@
#include <QTime>
#include <QtDebug>
#include <QQueue>
#include <qnetworkrequest.h>
Copy link
Member

Choose a reason for hiding this comment

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

just for consistency...

Suggested change
#include <qnetworkrequest.h>
#include <QNetworkRequest>


const QDir directory = QFileInfo( fileName ).absoluteDir();
mDirectory = directory.absolutePath();
QNetworkRequest nr = QNetworkRequest( QUrl( mUri ) );
Copy link
Member

Choose a reason for hiding this comment

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

It looks like in the previous implementation we have forgotten to add marker for the network request, e.g.

QgsSetRequestInitiatorClass( nr, QStringLiteral( "QgsEptPointCloudIndex" ) );

Could you please add it? (and one more when fetching manifest.json and one in loadManifest())

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.

2 participants