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

Request: Range check multiplier to get_range() as user-defined value #490

Closed
bi0m3trics opened this issue Oct 16, 2021 · 8 comments
Closed
Assignees
Labels
Feature request Asking for a new feature

Comments

@bi0m3trics
Copy link
Contributor

Hey JR - I have a feature request (maybe the easiest one ever) but it's one that makes lidR work for other sensor types (MLS in my case), however you may not want to open this door...

I've been using lidR to range correct the intensity of mobile lidar data, but to do so I have to go into LAS.cpp (lines 305-313) and remove (or creative an argument to bypass) the hard-coded range check multiplier (i.e., 3 * R_control) to make get_range() work where ranges can be highly variable (as is the case with terrestrial lidar). To be completely honest, I can just keep rebuilding my own version with each release and comment out these lines of code, but it would seem to be an easy fix if the user were allowed to supply their own "multiplier" (in my case 100 instead of 3, since I'm scanning parallel to the surface and not perpendicular to it as with ALS) and then the function would work across all instances.

Thanks!
Andrew

@Jean-Romain Jean-Romain self-assigned this Oct 17, 2021
@Jean-Romain Jean-Romain added the Feature request Asking for a new feature label Oct 17, 2021
@Jean-Romain
Copy link
Collaborator

That makes sense. However I don't want to add a complex argument difficult to understand. I prefer to do something more generic and transparent. I think I could change the threshold as a function of the point source. E.g. if you read las with readTLSLAS() the point cloud is flagged TLS and I could handle your case internally by disabling this test.

@bi0m3trics
Copy link
Contributor Author

bi0m3trics commented Oct 17, 2021 via email

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Oct 18, 2021

Your question actually raises more questions than we can think. In lidR all the range stuffs were designed for ALS but I do agree that it should work for others sources in some way. But.

  • What about TLS that have a single source point. Currently the algorithm would crash I think.
  • How so we retrieve the sensor location in TLS. AFAIK TLS is single return.
  • What about MLS, how did you get the sensor positions? From external file or with track_sensor()? Again I don't think track_sensor() can work. But get_range should if not limited by the internal check.

So I guess if should add an error in track_sensor() to abort computation in non aerial lidar + drop the test in get_range() for all non aerial lidar

@bi0m3trics
Copy link
Contributor Author

Easy answer first, most MLS on the market generate a file that logs the trajectory of the sensor (which is used by SLAM algorithms). Our Zeb Horizon provides this as a txt file with the time, x, y, and z of the sensor, which I just read in and convert to a SpatialPointsDataFrame (say, named traj). I then use that with get_range(las, traj) (and the modified LAS.cpp) to get the range and then correct the intensities using the same approach as Gatziolis 2011 to the tune of:
las@data$NormInt = las@data$Intensity * (las@data$range/median(las@data$range))^2). with f being around 2-3.
So for MLS , I agree that track_sensor() is not needed for MLS. For the record, I've not explored incident angle effects yet.

For TLS, well, I don't own one and have only received data from them second hand, so I'll have to ask around or we'll need someone else with more first hand experience to chime in... I agree that it's typically a single scan location with the gpstime varying among pulses, so I would also think get_range() would function similarly to that of MLS (i.e., one would never need track_sensor()).

Not 100% sure my responses here are helpful, hence the "you may not want to open this door" comment. If you want some MLS and trajectory data to play with just say the word, but I'm pretty sure you wont find it all that interesting. That is unless you're looking for another unfunded side project! ;)

@Jean-Romain
Copy link
Collaborator

It is ok

  • I'll disable the check for non aerial point sources (i.e. LAS read with e.g. readTLSLAS)
  • I'll fix the code for single sensor position.

It should do the job

@bi0m3trics
Copy link
Contributor Author

bi0m3trics commented Oct 18, 2021 via email

Jean-Romain added a commit that referenced this issue Oct 18, 2021
@Jean-Romain
Copy link
Collaborator

I disabled tests for TLS. I Considered MLS to be much like TLS than ALS so read it with readTLSLAS().

I did not fix code for single position sensor yet

@Jean-Romain
Copy link
Collaborator

I'm closing this. If somebody comes with TLS data and a problem for range normalization with single position sensor I'll do something using a reproducible example. But right now it is only an hypothetical problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Asking for a new feature
Projects
None yet
Development

No branches or pull requests

2 participants