-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Double precision for Data3D #126
Comments
Hmmm... I find the naming of these ( I think @ptc-jhoerner will have to answer this one. (I don't mean he has to, just that he's probably the one who can 😄 ) |
Having looked at this a little, I think this is a holdover from the original E57Simple that has been adapted by @ptc-jhoerner. Edit: This is wrong. See next comment. It looks like maybe the intent is to set
I think (Same issue with |
OK. After a bunch of experimentation, I think this is the situation... Set
@MCvin Does this fit with the problem you were having? Set it to -1.0 to use doubles. This is pretty confusing so I will rework this or at least document it better. |
I'm not sure since I don't understand what is pointRangeScale (or angleScale) and the global constant E57_NOT_SCALED_USE_FLOAT. I just saw that in my case the condition as it is written: My fix was to swap the result of the condition, i.e. |
That's what I'm trying to figure out/explain in my previous comment 😄 If you set |
Ah ah sorry I didn't give enough context maybe ^^ I use libE57Format as a library in another project so I was hoping using the class e57::Data3DPointsData_d with
That's why I thought maybe the if condition was incorrect and I prefer not to hardcode the value pointRangeScaledInteger. Does it makes any sense ? |
? I don't understand. The whole purpose of the If you set |
I never use
|
You do actually.
Look at your PointStandardizedFieldsAvailable pointFields; You have to be setting fields in it (or reading it from the file) in order for the library to read/write anything. It's how the library knows the available fields. (In the case of reading you don't get to choose whether you are using double, float, or scaled ints - it uses whatever the file contains. When writing you can choose to use double, float, or scaled ints - by setting
If you look at the writing example, the |
Oh ok, I will have to look into this. But it is strange since I use the same scan header as the original file which contains My test consist in reading and writing the same file, with the scanHeader exactly the same as the input I will have a look at the scanHeader and the value of pointRangeScaledInteger but my feeling is that something is strange since I just try to copy a file and use the exact same original header. |
Aha! I think this is the issue. I think reading and writing are - in a sense - asymmetrical and this won't work. I agree that it should though. If you look at So in your case what's probably happening is:
I looked at the original implementation. It stores everything as And looking at the history with this repo, I think this is what happened:
I'm not sure what the correct solution is for this, but I think not having to explicitly manage all of these details would be better. The original way of using One hacky way to do it is to use the template type to set Since I'm making other breaking changes for 3.0, I might rework all of this if I get inspired. (Also, you might find my last commit useful to require less boilerplate code and make some of this more robust.) |
Oh thank you for this feedback, it looks more complex than I thought initially. I will try what you suggest, thanks a lot! And good luck if you find the inspiration ^^ |
Me too 😆 I didn't write any of this - I'm just the Bit Janitor - so I have to work through it as well. The underlying API is flexible and the "simple" API was supposed build on it to provide a straightforward way to use it at the expense of that flexibility. Adding the double/float option to the Data3D pulls it back the other way and I think that's where the conflict lies. |
I agree the way For writing we need to know a bit more to support writing data with conversions, that is where this whole mess with |
Thanks Jiri!
I don't want to lose that functionality - in the tests I'm working on now, using scaled int results in file sizes 1/4 the size of the double one, so it's pretty useful!
I agree. I'm in the middle of writing up some tests for float/double/scaled so that I can make some more changes here. I think we can make it a little bit closer to symmetric read/write. (On read we need to set After that I will look at reworking how |
Thanks for looking into that! |
…reading doubles Simple fix, very long test! Related to #126
…reading doubles Simple fix, very long test! Related to #126
…reading doubles Simple fix, very long test! Related to #126
@MCvin I think I fixed the main problem you were reporting waayyyy up there. I added a rather long test in test/src/test_SimpleData.cpp that is meant to reproduce what you're doing. Please let me know if that's roughly what you have or if I'm missing anything. Before I merge it, could you please try the branch to see how it works for you? As outlined in my analysis, we don't handle all cases for all the scaled int fields, so we're still read/write asymmetrical... |
Thanks @asmaloney ! I was out for a few days so I didn't check yet, I will do it now. |
I just tried and your changes to master fixed it for my test case! Thank you so much! |
Excellent! I've been making a lot of other changes to simplify the use of the API and help prevent mistakes/errors - see the tests for examples. I have one more thing I want to do (automatically add some missing fields) then I'll let it sit for a while to wait for any feedback. |
libE57Format/src/WriterImpl.cpp
Line 750 in 50e84a2
libE57Format/src/WriterImpl.cpp
Line 786 in 50e84a2
Not sure if there is a mistake in these conditions but in my case I needed to swap E57_DOUBLE and E57_SINGLE to enable double precision in my point clouds:
The text was updated successfully, but these errors were encountered: