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

Add abstraction for managing different analysis methods #23

Conversation

philipbelesky
Copy link
Collaborator

This is very much a work in progress / for feedback PR. This sketches out a series of classes in Analysis.cs that are each tied to a particular analysis/visualization method, such as the existing elevation/blank options, future options such as #13, and color/gradient variations.

Each class should ideally:

  • Compute a color lookup table (this may not be strictly necessary for all analysis methods, but could be a useful way to handle gradient customisation cleanly).
  • Compute the relevant color for a given pixel. This is easy for the current elevation method, but would involve distinct logic for slope/aspect
  • Be able to store the variables used to present the analysis as an option in the context menu
    • Some analysis options form mutually exclusive sets (i.e. slope vs aspect vs elevation) while others could be toggled independently (contours vs water vs ??)

I'm not super experienced with proper composition/inheritance patterns in C# so thought I would put this out there to see if its roughly appropriate. Feel free to revise/finish/trash as necessary.

@mariuszhermansdorfer
Copy link
Owner

A very good idea, Philip! I like how you structured the whole thing. I'm not very experienced in this matter either, so I guess we'd have to experiment a bit to get it right.

For the sake of having an overview, a list of analysis that could be meaningful follows.

  1. Elevation
  2. Slope
  3. Aspect
  4. Aspect & Slope combined
  5. Concavity
  6. Hillshade
  7. Roughness
  8. Contour lines
  9. Water flow
  10. Watershed
  11. Cut & fill
  12. Heat map (colorize by user-defined values, i.e. groundwater data, highest amount of tweets at a location, sound wave distribution etc.).

Not saying we should implement all of these, but it's worth having a bigger picture when laying foundations for future work.

@mariuszhermansdorfer
Copy link
Owner

Good progress on this one today!
Do you think this is ready to be merged to master now?

@philipbelesky
Copy link
Collaborator Author

I’ll need to check it on a live setup as the above was done without access to a Kinect, but assuming that works I’ll mark it for a code review when its ready. I might have a look at the Color ramps branch and see if it can be integrated with that.

@philipbelesky
Copy link
Collaborator Author

Ok, I think this should be ready for review. A couple of notes:

  • I've used the braceless shorthand in a few places for one-liner if/else blocks. Wasn't where you come down on concision vs consistency on that front.
  • Each analysis method provides a range (or ranges) that map between values and a color interpolation. It should be relatively easy to allow these to be specified by the user - they would just shift to be initialized with the class.
  • For the non-elevation based visualizations there will need to be:
    • A way to pass an array of points to GetPixelColorForAnalysis; not sure if that should be handled with operator overloading / generics / or some other implementation that will allow each class to accept varying inputs.
    • A post-mesh-creation hook at the end of SolveInstance that will call into enabled options and allow them to output geometry (e.g. contours).
  • I wasn't sure how to easily check performance; the colors seem approximately what they were. Maybe its worth making a unit test with saved/randomly-generated point clouds so that can measure functions reliably despite different setups?

Copy link
Owner

@mariuszhermansdorfer mariuszhermansdorfer left a comment

Choose a reason for hiding this comment

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

Great work, Philip!
This lays a great foundation and provides tons of flexibility for the future!

@mariuszhermansdorfer mariuszhermansdorfer merged commit e41bdbb into mariuszhermansdorfer:master Sep 9, 2019
@philipbelesky philipbelesky deleted the feature/analysis branch September 12, 2019 08:50
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

2 participants