cesium-unreal
cesium-unreal copied to clipboard
Add Degree-Minute-Second editing support
This was only a "side task", because it bugged me that using the Latitude/Longitude sliders in the Editor UI caused the position to change far too quickly, and one had to type in the right value manually:
With this PR, in CesiumRuntime::StartupModule, we register a CesiumGeoreferenceCustomization. This establishes a special editing component for the "Details View" of Latitude/Longitude. This editing component is summarized in the main class of this PR, namely the CesiumDmsEditor.
(For more context, see the Unreal Docs about Details Customization. I might have violated some "best practices" here, *shrugs*. If somebody knows: "Nah, that's not how you should do this", feel free to chime in...)
The CesiumDmsEditor creates the Details View component that allows editing the property in two different ways:
- Using the default component, namely a
SpinBox<double>, for editing the actual property value in decimal degrees - Using three (+1) separate components for editing the same property, with spin boxes for Degrees, Minutes, and Seconds (+ the dropdown for N/S or W/E)
Note that this is only a user interface change: There is still only a single UPROPERTY to store the decimal degrees value. The UI components are just populated using the DMS representation that is computed from this property, or writing the value of the property based on the DMS representation that is edited in the UI.
A short summary of the current appearance and interaction:

(This is currently a draft: There are still some debug outputs, that will be removed iff this PR is about to be merged)
Thanks for the pull request @javagl!
- :heavy_check_mark: Signed CLA found.
- :grey_question: CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to
CHANGES.md.
- If this change updates the public API in any way, please add a bullet point to
Reviewers, don't forget to make sure that:
- [ ] Cesium for Unreal Samples works.
This looks great, let's get it merged.
I've marked it as "ready for review", but
before merging:
Such a review could cover the code-level as well as aspects of "usability" or UI/layout details. Right now, there's still log/debug output, but if there is no further feedback, I'd just remove that log output (or maybe make it LogCesium VeryVerbose or so...).
Yep I'll review the code and UI (both look good at a glance). Please remove the logging.
I have turned the log into LogCesium VeryVerbose and added some minor comments.
One thing that still carried a TODO was the line at https://github.com/CesiumGS/cesium-unreal/pull/646/commits/971773b0ad04329a5fe5f8d1a772462eb602237a#diff-d7d6af05c0f364914d90a60d1fa369d9831f0163f978390f926e90c359324f31R131 where I set the maximum value of the 'Seconds' slider to 59.999999, because it should offer the range [0,60) instead of [0,60] - there doesn't seem to be a "nice" way to achieve that (for obvious reasons). It should be OK, but hope that it does not cause any annoying glitches...
On a more conceptual side: The DMS struct and the degree<->DMS conversion functions could be (static private) in the header. I was a bit on the fence with that one: Although it would allow to avoid a few lines of redundant code in the CPP file, this is actually only an implementation detail - and even though the class already is in the Private folder, I thought it could make sense to treat it as such, and put it into the CPP for that reason...
Would it be possible to put all of this code in CesiumEditor instead of CesiumRuntime? It might be more trouble than it's worth, but at a glance it looks like it might be very straightforward, in which case I think it's well worth doing.
I'll have to take a look at (or just try out) the registration of the editor components that is currently done in CesiumRuntime.cpp at https://github.com/CesiumGS/cesium-unreal/pull/646/files#diff-410b2b68ed714723d92217e76b486fb234cdd08139c62c3f29f0ca02011f871aR32 . A very vague concern is that ~"if it is not done correctly, then it might cause some odd dependency from the packaged game to the editor part, which should not be there". But I'll give it a try, and if it's "wrong", this will likely be caught during the review.
Totally unrelated (well, to this PR, but not to the reason for this PR), just to have the link (from 2017) here: https://answers.unrealengine.com/questions/566915/view.html
Is there some compelling purpose that isn't obvious to me?
Possible user posts/issues like "I entered 123, but it shows 123.0001, why?". But maybe that's not a strong reason for now.
Would it be possible to put all of this code in CesiumEditor instead of CesiumRuntime? It might be more trouble than it's worth
That certainly makes sense, considering that basically all classes had been wrapped into some (ridiculously global) #if WITH_EDITOR anyhow. So now it's part of the CesiumEditor. The other inlined comments should be addressed as well.
One last thing that bugs me (far more than it should) is that the "Place Georeference Origin Here" button is still shown as the last component, even though it shouldn't. To be clear: It does not bug me that it is shown as the last component (because that doesn't matter so much). It bugs be that it is shown as the last component even though it is explicitly and dedicatedly added as the first one, and 1. every source that I found said that the properties will be shown in the order in which they are added, 2. that's what one would expect (I mean, what else, seriously?!?), 3. there is no obvious way to affect the order otherwise. If somebody has a hint how to fix that without reading engine code for a few hours, then I'll do that, but I already spent more time than I could reasonably justify for this detail...
One last thing that bugs me (far more than it should) is that the "Place Georeference Origin Here" button is still shown as the last component, even though it shouldn't.
I looked into this briefly, and the problem is that AddProperty can't be used with a UFUNCTION. That call has no effect whatsoever. I think that a custom UI needs to be added for the button, and then the UFUNCTION needs to be excluded from being added automatically at the bottom by removing its EditAnywhere attribute where it's declared. It's really frustrating that there's no AddFunction equivalent to AddProperty, but I don't see aything of the sort.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
I wonder if this had been merged if there wasn't that issue of the "Place Georeference Origin Here" button.
I'll probably disable the concierge notifications soon. But I think that this editor really could have increased the usability.
Thanks again for your contribution @javagl!
No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?
I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
Closing this pull request, as it's been over a year since activity. Will create an issue (#1137) to remind us that there is functionality here worth integrating.