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

Fast aoi queries #2

Merged
merged 7 commits into from
Feb 3, 2017
Merged

Conversation

floriandeboissieu
Copy link
Contributor

Changed lasfilter queries for lasreader fast AOI queries, which makes the extraction of rectangle/circle faster with indexed las/laz files (see link).

Copy link
Collaborator

@Jean-Romain Jean-Romain left a comment

Choose a reason for hiding this comment

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

Thanks, but

  1. You must explain to me what you did. We already discussed about the frame, so I know what is the purpose. But I do not understand some specific points
    • I added notes in your PR.
    • What happens if there is no LAX file? It reads the file in the standard way? Even with a filter command?
    • What happens if there is a LAX file?
    • Does the reader automatically detect the LAX file?
    • Many other question. Please explain me the worflow.
  2. There is 1 bad thing in your commit and 1 I'm not comfortable with
    • Having two commits in one PR is not good you should squash all your commits together. But actually I don't care of that. My concern is about the fact that my previous commits and the Dave's one are included in your PR. I don't know who made something wrong but that does not make sense.
    • I'm not comfortable with the fact you modified LASlib. I did it but only to make it R compliant. Here you added a function. We can do that, the licence allows us to do that, so no problem and it makes a lot of sense. But... Well I don't know why I don't like that.

@@ -23,7 +23,7 @@
implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

CHANGE HISTORY:

3 February 2017 -- by Florian de Boissieu -- L1798,1813 added parse_str because of strange problem with overloaded functions
Copy link
Collaborator

@Jean-Romain Jean-Romain Feb 3, 2017

Choose a reason for hiding this comment

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

I know that there is a strange problem with overloaded functions. I'm ok with that. But why you needed a new function. There is no function to parse CHAR?

src/readLAS.cpp Outdated

npoints = 0;
while (lasreader->read_point())
{
if(!lasfilter.filter(&lasreader->point))
// if(!lasfilter.filter(&lasreader->point))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It means the reader skip automatically the points based on the filter which is now into the reader. Right?

src/readLAS.cpp Outdated
@@ -142,8 +144,8 @@ List lasdatareader(CharacterVector file, bool Intensity, bool ReturnNumber, bool
unsigned long int i = 0;
while (lasreader->read_point())
{
if(stream && lasfilter.filter(&lasreader->point))
continue;
// if(stream && lasfilter.filter(&lasreader->point))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw same comment than the previous one.

src/readLAS.cpp Outdated
LASreader* lasreader = lasreadopener.open();
LASfilter lasfilter;
// LASfilter lasfilter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It means the filter is integrated into the reader. Right?

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Feb 3, 2017

I don't know who made something wrong but that does not make sense.

I'm guilty. master and devel were not up to date. Moreover Dave pushed in master instead of devel. So it was messy. master and devel are now even. Update your fork.

Edit: you're guilty too. You worked in master an pushed in devel.

@Jean-Romain Jean-Romain merged commit b4b3b72 into r-lidar:devel Feb 3, 2017
Jean-Romain pushed a commit that referenced this pull request Dec 31, 2018
changed lasfilter queries with lasreader fast AOI queries


Former-commit-id: b4b3b72
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

3 participants