Skip to content

Improve working dynamically with units and quantities #576

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

Merged
merged 39 commits into from
Feb 1, 2019

Conversation

angularsen
Copy link
Owner

@angularsen angularsen commented Dec 16, 2018

Motivation

Make it easier to parse, convert and enumerate quantities and units dynamically without having to use reflection.

Changes

  • Add QuantityInfo to encapsulate metadata about a quantity type, such as names, units, conversion functions
  • New Quantity class for exposing the majority of dynamic helper functions
    • string[] Names, QuantityType[] Types, QuantityInfo[] Infos
    • QuantityInfo GetInfo(QuantityType)
  • UnitConverter: Add Convert(QuantityValue, Enum, Enum) and TryConvert methods, was useful in sample apps
  • Quantities
    • Add As(Enum) and ToUnit(Enum) methods for dynamic conversion
    • Add instance getters for QuantityInfo exposed via IQuantity
    • Add static getter for QuantityInfo (Length.Info)
  • Remove most reflection code usage
  • Update sample apps to not use reflection

See PR's README dynamic parsing/conversion section for some example code.

@angularsen angularsen force-pushed the add-unitshelper branch 2 times, most recently from 2016559 to ae3c904 Compare December 16, 2018 13:17
@angularsen
Copy link
Owner Author

@tmilnthorp Would love your review on this when you have the time. No rush!

@tmilnthorp
Copy link
Collaborator

I like it! Having both ways is very helpful.

@angularsen
Copy link
Owner Author

Cool, I'll look into adding both then.

@angularsen angularsen force-pushed the add-unitshelper branch 3 times, most recently from c197b47 to d71f373 Compare January 27, 2019 23:30
Adding BaseDimensions to QuantityInfo
It was not a good idea to use extension methods with similar names as some of the targets.
A wrapper type avoids this problem.
@angularsen
Copy link
Owner Author

@tmilnthorp All compiles and tests are green, would you like to review this further before I merge?

@angularsen
Copy link
Owner Author

Bwt #591 is merged in here, so merging this would also merge that.

@tmilnthorp
Copy link
Collaborator

Sure let me look through.

Adding BaseUnit to QuantityInfo. Adding GetQuantitiesWithBaseDimensions helper to Quantity class
Adding Unit to IQuantity
@tmilnthorp
Copy link
Collaborator

I added a couple minor things.

BTW it's hard to ignore the increasing number of things we can't do in WRC. #if !WINDOWS_UWP keeps creeping throughout the code.

@angularsen
Copy link
Owner Author

angularsen commented Feb 1, 2019

I've been soo frustrated with WRC the entire v4 cycle and now again this PR with a bunch of generics and reflection that don't sit well with WRC. This PR took at least twice the time, if not more, due to WRC.

I'm seriously considering branching out WRC to its own separate thing. Maybe its own repo even. It can share .json files maybe, but other than that its entirely own script files and source code. I mean, WRC as a framework I suspect is pretty much end of life in terms of support and new features. I don't have any numbers, but all I've read about it is the failed promise of allowing JS and C++ developers write UWP apps. And who writes UWP apps anyway. It's dead, in my eyes.

We do have some users, not many, some tens of people/teams. It's hard to tell from nuget statistics. Recent versions are getting 40-50 downloads, so those are at least actively pulling new versions.
Some old versions have had as many as 3,688 downloads, but with CI builds that refresh their cache and many members and machines on teams etc, it doesn't really paint a good picture.

The guy who originally asked for it, is not updating their libraries either:

Microsoft.IoT.Devices by: jbienzms
Devices for Windows IoT Core
 9,686 total downloads  last updated 2/7/2017  Latest version: 1.0.8  sensors devices hardware displays 

And these seem just as old:

microsoft windows iot iot-core
Makes it easy to use sensors, displays, and other hardware devices on the Windows IoT Core platform.

Windows.IoT by: tdecroyere
 1,279 total downloads  last updated 3/15/2015  Latest version: 1.0.0-alpha-4

I may be a bit blinded by all the recent pain with WRC in this PR, but I currently feel we should leave it behind on version 4.4.0 and branch out the code - for our own sanity. It's a big friction to development and maybe especially to contributors, to have to deal with this 1GB addon to Visual Studio just to even compile the thing, let alone it being a separate solution and something always goes wrong with it.

Arrrrgh! So much pain! :-)

I'm pinging @jbienzms to let him chime in on the topic if he wants to.

Update:
I just realized, the majority of pain is adding new code features. Adding new units and quantities is still a breeze in WRC, so what we need to do is split out the code generator scripts for WRC and get its full copy of all the shared source code, then we can still add new units to both UnitsNet and WRC and produce new versions as before - it just won't get things like Quantity and QuantityInfo in the future.

@angularsen
Copy link
Owner Author

angularsen commented Feb 1, 2019

Merging this then.
@tmilnthorp any input on the WRC approach I outline above? If you agree on separating it I'd love to start on that sometime soon.

@tmilnthorp
Copy link
Collaborator

Merge away!

I agree about WRC. Let's see if we get any input first. Maybe create an issue?

@jbienzms
Copy link

jbienzms commented Feb 4, 2019

I'm still at Microsoft but I'm in a very different role than I was when I created the IoT library. These days I'm working with HoloLens and Mixed Reality, so I spend much of my time in Unity. (Though just as an FYI, Windows Mixed Reality and especially HoloLens apps still compile to the Universal Windows Platform.)

Just out of my own curiosity and fascination, what are the major pain points of working with WRC? I think it was @angularsen that said the latest PR took several times the amount of effort? I'd just be interested to know what is adding so much complexity.

I can't say anything about Windows IoT Core because I haven't used it in almost 2 years. But I can say that HoloLens apps under UWP can take advantage of .Net 4.6 libraries as well as .Net Standard 2.0 libraries. Libraries don't have to be Windows Runtime Components.

If I were starting to develop a library like UnitsNet today, I personally would target .Net Standard 2.0. Depending on the level of complexit, I might also have builds that work with .Net Standard 1.0 and .Net 4.6 to increase coverage. Obviously things are a little different for you guys since you have existing consumers to support, but that is personally what I would target today if starting new.

Sorry I was never able to do more with UnitsNet back in the IoT days. But interestingly enough, I may have an opportunity to start leveraging it agian. Especially for distance conversions. (Don't want to hijack the conversation but I'm currently looking into converting between Geodetic and ECEF for Augmented Reality. You can see that project here.)

Best of luck guys. Thanks for all your hard work and for sharing it with the world.

@jbienzms
Copy link

jbienzms commented Feb 4, 2019

P.S. Just for reference (and for anyone who stumbles onto this thread), I wanted to point out that there's a difference between "UWP Library" and "Windows Runtime Component". I think that gets confusing here because above your conditional compilation statements are #if !WINDOWS_UWP but as I understand it the problems are likely due to Windows Runtime Component restrictions.

The Universal Windows Platform has a very broad set of APIs. It started as .Net 4.5 + some great stuff for cameras, media, etc. The UWP has evolved over time to support .Net 4.6, .Net Standard 1.0, .Net Standard 2.0 and beyond. The set of APIs available under UWP is immense.

Windows Runtime Components WRC were originally created as a way of writing one library that can be consumed by C++, C# and JavaScript. The number of APIs that are available in WRC is much smaller because it is an environment that is designed to be portable. Not only portable across .Net verions, but portable across languages. It's the latter, language portability, that causes so much of the complexity in building WRCs.

Having UnitsNet be a WRC does allow it to be used by C++ and JavaScript in addition to C#. However, this only works for applications that run under UWP. It's probably nice to have language portability, but saying "We run on JavaScript, under UWP" obviously isn't the same as just saying "We run on JavaScript". The surface area is much smaller.

That's why, today, I personally recommend some flavor of .Net Standard. .Net Standard is the new standard for portability across .Net Runtimes and even operating systems. You can run .Net Standard on Linux, for example. I think being here will give you more portability and bigger reach.

I still think UWP is a great platform. Especially for the convenience APIs around things like Media, Camera, Encryption, Networking, etc. I still think there are many UWP developers and apps that can benefit from UnitsNet. But I don't think UnitsNet needs to be a WRC these days to meet the needs of UWP developers.

Hope that helps clear things up a bit. Thanks again for building this library.

@angularsen
Copy link
Owner Author

Thanks for jumping in @jbienzms , I think we are pretty much on the same page here.

Just out of my own curiosity and fascination, what are the major pain points of working with WRC?

I think it comes down to:

  1. Very strict/limited public API when targeting WRC - there is a large number of constraints to satisfy on the public API of UnitsNet
  2. Very few use it or know what it is, so it's a big unknown for most contributors when AppVeyor build fails with a weird compile error they have to research - big friction for onboarding new contributors
  3. Needs UWP development workload installed in Visual Studio to compile locally, which last time I checked was in the order of 1GB to install and something most contributors don't already have.
  4. Code is littered with #if WINDOWS_UWP statements, or equivalent in our PowerShell code generator scripts, making it hard to maintain and extend - especially due to point 1. I know the naming WINDOWS_UWP is not accurate and the distinction between WRC and UWP, but I believe it was maybe inspired by some official code examples at the time. Don't recall.

All in all, it just adds pain to add/change code that otherwise would be trivial to modify.

I was on the fence on whether we should just drop WRC or if we should make an effort to keep it alive for adding new units to it, but in light of reading your personal opinion on the state of WRC and having given it some more thought myself, I am inclined to dropping it and rather add it back if someone comes asking for it. Then those who need and use it can spend the effort to keep it up to date.

I think we're talking at most some tenfold of teams actively using the WRC target. We already target netstandard 2.0 and net40 and as you say WRC really only adds support for JavaScript and C++ UWP apps. I'm guessing the few happy devs on that target are happy with the quantities and units already there, if not, they can help out 😄

@angularsen
Copy link
Owner Author

By the way @jbienzms , Unity is awesome! I have been dabbling with Oculus and VR myself the past year. If only I could get a hold of a Hololens unit! 🤤 Heard good things about it, but not tried it myself yet. It needs to get a lot more affordable 😞

@jbienzms
Copy link

jbienzms commented Feb 4, 2019

in light of reading your personal opinion on the state of WRC

Well, please keep in mind that I am not on the WRC team nor am I on the IoT team. And I really want it to be clear that this is just my personal opinion about what I would do if I was starting a new project like UnitsNet.

I do believe that WRC still provides a lot of value for for a library that:

  1. Targets UWP only
  2. Wants to support all UWP langages (C++, C# and JavaScript)
  3. May want brokered access out of the UWP sandbox for enterprise apps

I don't personally feel that WRC is exceedingly helpfu for a library like UnitsNet that:

  1. Is written in C#
  2. Is consumed virtually entirely by C# apps
  3. Runs in multiple environments (most of which are not UWP specific)

If WRC wasn't causing a lot of headaches I'd encourage you to leave it in. But based on the complications you've mentioned and the stats on your customer segments, I don't personally see it giving you the right benefit for the effort.

Leaving WRC behind does drop support for C++ and JavaScript, but that only worked for apps targeting UWP. UnitsNet never did work for "regular" C++ and JavaScript apps outside of UWP. You guys obviously know your customer base far better than I do, so you know how how many people will be impacted by dropping C++ and JavaScript UWP suppport. But you can always make a branch of the current codebase called "WRC Support" and leave that branch as it is. If you ever end up having customers that need new features added to the WRC branch, they can hopefully help to migrate those features into that branch.

Ultimately you'll have to decide on the future direction of the library. I would love to see the current version of the WRC component remain available in NuGet, even if you stop developing WRC support. And I'd like to see the "WRC Support" branch I mentioned above. But if you don't have many customers using WRC, I wouldn't fault you at all for removing it it and focusing on .Net Standard going forward.

Unity is awesome! I have been dabbling with Oculus and VR myself the past year.

Oh man, that's awesome. Let me know if you ever want to chat about developing in this environment. I find it both fascinating and really rewarding.

Take care!

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.

3 participants