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

finish up Walls import #90

Closed
25 tasks done
jedwards1211 opened this issue Dec 1, 2015 · 52 comments
Closed
25 tasks done

finish up Walls import #90

jedwards1211 opened this issue Dec 1, 2015 · 52 comments

Comments

@jedwards1211
Copy link
Contributor

These are checklists for myself:

  • add back ability to import .srv files directly
  • make it possible to pass in Walls for all instances of Survex shown on screen in cwImportSurvexDialog (haven't completely figured out how to work within the constraints of the form designer yet)
  • use .NAME directives in .wpj files for trip titles
  • refactor to disable cwSurvexGlobalData's station renaming for Walls import only

Check that the following work properly (ideally, with unit tests):

  • .PATH directives in .wpj files

  • calibrations/team members

  • #units save and #units restore

  • instrument/target heights and inch parsing

  • deriving distance and inclination from inst/target heights, inch correction, and taping methods

  • cd-like behavior of #segment directives

    Reverse-engineered spec:

    • .. and . can only come at beginning of path
    • / and \ are treated identically
    • path elements may not begin with . (except initial .. and . elements)
    • path elements may not contain /, \, or ;
    • trailing whitespace is removed from last path element FIX THIS -- parser currently trims on both sides
    • if the path begins with / or \ it resets to the name-defined segment from the .wpj file
    • .. beyond root does nothing
  • incs correction of lruds/ih/th

Double check if Walls supports:

  • ; comments at the end of lines in .wpj files -- nope
  • the same station name restrictions as dewalls does
    Nice. Walls gives invalid errors when TO stations start with < (which is ambiguous with LRUDs)
  • too many .ENDBOOK directives -- ignores the rest of the file
  • too many ../ in #segment directives yup
  • absolute .PATHs of any kind -- yup, support implemented

Process the following if necessary:

  • preserve length/orientation of vertical shots flags -- Walls adjusts variance according to these, unless survex supports variance overrides we have no use for these
  • name defines segment flag
  • derive declinations from #date flag (will defer this)
  • UTM/GPS grid relative flag -- not necessary, this is for Walls output coordinates only
  • grid convergence setting from .wpj files (same as #units grid=...?) just affects Walls output coordinates and maps. Will be important for importing Walls calculated coordinates though!

Display warnings when:

  • #fix stations are present
  • stations have to get renamed
  • any entries have derive decl from date checked
  • move these warnings to the import warnings tab
@jedwards1211 jedwards1211 changed the title finish implementing/merging Walls import finish up Walls import Dec 1, 2015
@jlillest
Copy link
Contributor

jlillest commented Dec 1, 2015

nice!

@vpicaver
Copy link
Contributor

vpicaver commented Dec 1, 2015

Sweet!

I didn't know you could make checkboxes in github, that's pretty cool.

@jedwards1211
Copy link
Contributor Author

Yeah, that way we don't have to make a bajillion tiny issues for things

@jedwards1211
Copy link
Contributor Author

@vpicaver do you know what library you want to use for computing declination automatically?

@vpicaver
Copy link
Contributor

vpicaver commented Dec 5, 2015

Haven't researched it much. Do you have one in mind?

On Saturday, December 5, 2015, Andy Edwards notifications@github.com
wrote:

@vpicaver https://github.com/vpicaver do you know what library you want
to use for computing declination automatically?


Reply to this email directly or view it on GitHub
#90 (comment).

@jlillest
Copy link
Contributor

jlillest commented Dec 5, 2015

In case it helps, here's the NOAA page for info on third-party software for the World Magnetic Model(WMM):
http://www.ngdc.noaa.gov/geomag/WMM/thirdpartycontributions.shtml

@vpicaver
Copy link
Contributor

vpicaver commented Dec 6, 2015

http://geographiclib.sourceforge.net/html/classGeographicLib_1_1MagneticModel.html

This might be some that could work. We would need to create a qbs build of
it so it integrates with our qbs / sub module build system. Currently it
uses cmake.

On Saturday, December 5, 2015, jl <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

In case it helps, here's the NOAA page for info on third-party software
for the World Magnetic Model(WMM):
http://www.ngdc.noaa.gov/geomag/WMM/thirdpartycontributions.shtml


Reply to this email directly or view it on GitHub
#90 (comment).

@jedwards1211
Copy link
Contributor Author

Okay, cool. I love how many good libs like that are available for C++. Hopefully I'll get to that soon...just did a massive code cleanup today

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

Yea it seems promising, I'm sure there is others out there.

On Sunday, December 6, 2015, Andy Edwards notifications@github.com wrote:

Okay, cool. I love how many good libs like that are available for C++.
Hopefully I'll get to that soon...just did a massive code cleanup today


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Any tips on how I can make my Length and Units singletons thread-safe? They're getting initialized twice in a test...

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

So it's calling isNull() returning true both times?

On Sunday, December 6, 2015, Andy Edwards notifications@github.com wrote:

Any tips on how I can make my Length and Units singletons thread-safe?
They're getting initialized twice in a test...


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

Oh don't need to use a shared ptr for _type in length.h. Just make _type a
normal pointer and initialize it in define it statically in length.cpp

Const Length* Length::_type = new Length();

You need to make _type Const in the header as well. Then you won't need an
init() function because it will be initialized when the static section is
created when the program starts up.

I don't have my computer until Tuesday so I can test or try stuff out.

On Sunday, December 6, 2015, Philip Schuchardt vpicaver@gmail.com wrote:

So it's calling isNull() returning true both times?

On Sunday, December 6, 2015, Andy Edwards <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Any tips on how I can make my Length and Units singletons thread-safe?
They're getting initialized twice in a test...


Reply to this email directly or view it on GitHub
#90 (comment)
.

Phi|ip

Sent from Gmail Mobile

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Actually, I just found out about Q_GLOBAL_STATIC, I'm going to try it out...

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

Interesting, didn't know about. Looks useful

On Sunday, December 6, 2015, Andy Edwards notifications@github.com wrote:

Actually, I just found out about Q_GLOBAL_STATIC, I'm going to try it
out...


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Weird. Even when I used Q_GLOBAL_STATIC my Length constructor gets called twice (according to print statements I added) when running the tests.

Another weird thing is that when I try to run the tests in debug mode, I can't get it to break or print anything, but when I run the main app in Debug mode, it works just fine. I wonder if Catch has something to do with that.

@jedwards1211
Copy link
Contributor Author

Ah, problem with debugging in test mode was that I had run in console checked. When I unchecked it and it ran in the IDE console, I got it to break :)

@jedwards1211
Copy link
Contributor Author

Agh. The double-loading is happening because one access to the singleton is triggered by other statics. Singletons are so difficult in C++...people are so critical of them, but this really is a logical use case

@jedwards1211
Copy link
Contributor Author

I think if I load some global constants in WallsParser and CardinalDirection with a Q_GLOBAL_STATIC as well, the problem will resolve itself

@balister
Copy link

balister commented Dec 7, 2015

Hippies.

On 12/06/2015 06:40 PM, Andy Edwards wrote:

I think if I load some global constants in WallsParser and CardinalDirection with a Q_GLOBAL_STATIC as well, the problem will resolve itself


Reply to this email directly or view it on GitHub:
#90 (comment)

@jedwards1211
Copy link
Contributor Author

The way I was doing units was so elegant in Java. I guess I'll have to do something different in this code

@jedwards1211
Copy link
Contributor Author

Alright! I refactored the units stuff to get rid of the singletons. At the time I wrote them, I wasn't familiar enough with C++ templates to know how to use them in this case

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

Cool I'm glad you figured it out

On Monday, December 7, 2015, Andy Edwards notifications@github.com wrote:

Alright! I refactored the units stuff to get rid of the singletons. At the
time I wrote them, I wasn't familiar enough with C++ templates to know how
to use them in this case


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@vpicaver
Copy link
Contributor

vpicaver commented Dec 7, 2015

Isn't there a singleton keyword in Java?

On Monday, December 7, 2015, Philip Schuchardt vpicaver@gmail.com wrote:

Cool I'm glad you figured it out

On Monday, December 7, 2015, Andy Edwards <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Alright! I refactored the units stuff to get rid of the singletons. At
the time I wrote them, I wasn't familiar enough with C++ templates to know
how to use them in this case


Reply to this email directly or view it on GitHub
#90 (comment)
.

Phi|ip

Sent from Gmail Mobile

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Nah, but there are some pretty simple ways to make them, and there's no load-ordering problem with static member variables, the VM makes sure they load in the proper order if one references another (even if they're in different classes)

@jedwards1211
Copy link
Contributor Author

On the other hand, C++ templates are awesome and proved to be a totally better solution for how I'm using units

@jedwards1211
Copy link
Contributor Author

Here's a singleton for you guys, right in the middle of GeographicLib: http://geographiclib.sourceforge.net/html/classGeographicLib_1_1Geocentric.html#af6e84e6d9d7967df6ba13cae1814cb7b

@jedwards1211
Copy link
Contributor Author

Hmmm, calculating declination automatically from data imported from Walls will take quite a bit of work, because Walls supports a pretty wide variety of reference datums. Would you guys rather wait for me to implement all of that before a release, or have me just show import warnings if the walls data is set up to calculate declination automatically, and fully implement that later?

@jlillest
Copy link
Contributor

jlillest commented Dec 8, 2015

Walls is setup to calculate declinations from geo reference automatically, so I suspect not having a static declination in project files will be the norm. That being said, I think pushing for an intermediate release where declinations are pegged at zero when not specified otherwise is fine. Then the datums can be implemented one at a time, I suppose. WGS84/NAD83 would be the first one to do, followed by NAD27 to get the majority of US users.

@jlillest
Copy link
Contributor

jlillest commented Dec 8, 2015

If it were me, I'd add an import warning that says something like: "Datum not currently supported, please file a feature request on github". :D

@jedwards1211
Copy link
Contributor Author

Yeah, I'm fine with that. Phi|ip?

@vpicaver
Copy link
Contributor

vpicaver commented Dec 8, 2015

Warning is fine for now. 😀

On Monday, December 7, 2015, Andy Edwards notifications@github.com wrote:

Yeah, I'm fine with that. Phi|ip?


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Cool.

What's the equate map stuff for? I discovered it adding x's to the end of my stations unnecessarily (for Walls import, at least), so I'll have to do some kind of refactoring to turn off that behavior for Walls import. Perhaps extract a base class from cwSurvexGlobalData?

@jedwards1211
Copy link
Contributor Author

The new warnings/errors in the data pages are really neat though!

@vpicaver
Copy link
Contributor

vpicaver commented Dec 8, 2015

The equate stuff is for survex imports. To connect stations between trips
you must export and equate them. You can also alias stations using them.

Check out the survex documentation:
http://survex.com/docs/manual/datafile.htm

On Tuesday, December 8, 2015, Andy Edwards notifications@github.com wrote:

Cool.

What's the equate map stuff for? I discovered it adding x's to the end of
my stations unnecessarily (for Walls import, at least), so I'll have to do
some kind of refactoring to turn off that feature for Walls import. Perhaps
extract a base class from cwSurvexGlobalData?


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Oh, weird. No way to mark the trip blocks as not adding a prefix to the station names within them, huh? In Walls it's more explicit, on one hand there are these #segments that can follow the tree structure, but can be overridden, and those don't affect uniqueness (and they serve no purpose for import to Cavewhere right now anyway) -- only prefixes, of which there can be 3 and manually set only, affect uniqueness

@jedwards1211
Copy link
Contributor Author

Sweet, just got the refactoring done, wasn't too hard

@jedwards1211
Copy link
Contributor Author

It was:

  • rename cwSurvexImportDialog => cwImportTreeDataDialog
  • rename cwSurvexBlockData => cwTreeImportDataNode
  • extract a base class from cwSurvexGlobalData named cwTreeImportData, and make its caves() a pure virtual function
  • create cwWallsImportData that extends cwTreeImportData
  • use cwSurvexGlobalData in cwSurvexImporter and cwWallsImportData in cwWallsImporter

cwTreeImportDataNode still contains some Survex import-specific things. Do you want me to clean that up too? That would involve making it a base class and having cwSurvexBlockData extend it.

@vpicaver
Copy link
Contributor

That's a good question. How long would it take to make cwSurvexBlockData as
a sub class? If you think it'll be easier to understand, do it. :D

Phi|ip

On Fri, Dec 11, 2015 at 6:55 PM, Andy Edwards notifications@github.com
wrote:

It was:

  • rename cwSurvexImportDialog => cwImportTreeDataDialog
  • rename cwSurvexBlockData => cwTreeImportDataNode
  • extract a base class from cwSurvexGlobalData named cwTreeImportData,
    and make its caves() a pure virtual function
  • create cwWallsImportData that extends cwTreeImportData
  • use cwSurvexGlobalData in cwSurvexImporter and cwWallsImportData in
    cwWallsImporter

cwTreeImportDataNode still contains some Survex import-specific things.
Do you want me to clean that up too? That would involve making it a base
class and having cwSurvexBlockData extend it.


Reply to this email directly or view it on GitHub
#90 (comment).

@jedwards1211
Copy link
Contributor Author

probably wouldn't take too long, but I may or may not finish stuff up this weekend. I'm really freaking close though :)

@vpicaver
Copy link
Contributor

No worries. Going caving :D

On Friday, December 11, 2015, Andy Edwards notifications@github.com wrote:

probably wouldn't take too long, but I may or may not finish stuff up this
weekend. I'm really freaking close though :)


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@vpicaver
Copy link
Contributor

If you get it done next week, we can probably release a new version.

Phi|ip

On Fri, Dec 11, 2015 at 7:33 PM, Philip Schuchardt vpicaver@gmail.com
wrote:

No worries. Going caving :D

On Friday, December 11, 2015, Andy Edwards notifications@github.com
wrote:

probably wouldn't take too long, but I may or may not finish stuff up
this weekend. I'm really freaking close though :)


Reply to this email directly or view it on GitHub
#90 (comment)
.

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

actually, using inheritance in trees of interrelated types has always been awkward...it would be pretty nice in JS for example, I could easily tack the survex-specific data onto the tree nodes and process it later. Any idea of a good C++ way to do that using a QMap<QString, QVariant> or something like it?

@jedwards1211
Copy link
Contributor Author

actually, what am I thinking...I can just do

template<typename T>
class cwTreeImportDataNode {
  ...
  T UserData;
}

@jedwards1211
Copy link
Contributor Author

wait a minute, no I can't...damn non-generic templates...

@jedwards1211
Copy link
Contributor Author

I ended up just using a QHash<cwTreeImportDataNode*, cwSurvexNodeData*> in cwSurvexBlockData

@jedwards1211
Copy link
Contributor Author

God damnit VC++...

C:\Users\andy\build-cavewhere-Desktop_Qt_5_5_1_MSVC2013_64bit-Release\qtc_Desktop__73601786-release\cavewhere-test.qtc-Desktop--73601786.6720c1df\cwWallsImporterTest.cpp.obj:-1: error: LNK2019: unresolved external symbol "public: static void __cdecl cwWallsImporter::importCalibrations(class dewalls::WallsUnits,class cwTrip &)" (?importCalibrations@cwWallsImporter@@SAXVWallsUnits@dewalls@@AEAVcwTrip@@@z) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____12(void)" (?____C_A_T_C_H____T_E_S_T____12@@yaxxz)

I added a missing const to the cpp file, but it's still giving me the linking error. I don't see what I could possibly be doing wrong

@vpicaver
Copy link
Contributor

Is cwWallsImporterTest in a dynamic library? On windows you have to export
the class symbols if you're using in another place, like the testcases.
There should be other examples inside of the current master like cwCave.

Phi|ip

On Sun, Dec 13, 2015 at 8:22 PM, Andy Edwards notifications@github.com
wrote:

God damnit VC++...

C:\Users\andy\build-cavewhere-Desktop_Qt_5_5_1_MSVC2013_64bit-Release\qtc_Desktop__73601786-release\cavewhere-test.qtc-Desktop--73601786.6720c1df\cwWallsImporterTest.cpp.obj[image:
👎] error: LNK2019: unresolved external symbol "public: static void
__cdecl cwWallsImporter::importCalibrations(class dewalls::WallsUnits,class
cwTrip &)" (?importCalibrations@cwWallsImporter@@SAXVWallsUnits@dewalls
@@AEAVcwTrip@@@z https://github.com/Z) referenced in function "void
__cdecl ____C_A_T_C_H____T_E_S_T____12(void)"
(?____C_A_T_C_H____T_E_S_T____12@@yaxxz)

I added a missing const to the cpp file, but it's still giving me the
linking error. I don't see what I could possibly be doing wrong


Reply to this email directly or view it on GitHub
#90 (comment).

@jedwards1211
Copy link
Contributor Author

Ah, cool, thanks, I don't think I ever would have figured that out!

@vpicaver
Copy link
Contributor

Yea the joys of Windows....

On Monday, December 14, 2015, Andy Edwards notifications@github.com wrote:

Ah, cool, thanks, I don't think I ever would have figured that out!


Reply to this email directly or view it on GitHub
#90 (comment).

Phi|ip

Sent from Gmail Mobile

@jedwards1211
Copy link
Contributor Author

Any idea how I can dynamically change the text of the Survex Errors label? If so you can get points on my SO question: [[http://stackoverflow.com/questions/34298208/dynamically-change-text-of-tab-created-using-qt-designer]]

@jedwards1211
Copy link
Contributor Author

we can close this now

@vpicaver
Copy link
Contributor

:D

On Wed, Jan 13, 2016 at 11:02 AM, Andy Edwards notifications@github.com
wrote:

we can close this now


Reply to this email directly or view it on GitHub
#90 (comment).

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

No branches or pull requests

4 participants