-
Notifications
You must be signed in to change notification settings - Fork 73
Update API from float to double #126
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
Conversation
|
I considered an "intermediate" step here, with the attempt to retain a bit of backward compatibility. That intermediate step would be:
But I'm leaning slightly towards "ripping the bandaid off quickly": No matter how it is done, this will be a breaking change. And I think that when the change is breaking anyhow, then the new state should preferably be "clean". As mentioned in the first comment: The changes for clients will be fairly trivial (even though they may affect many places). |
|
Hi! In general I agree with your Idea of "ripping the bandaid off quickly". Are there any other breaking changes suggested or in the pipeline that would be released in 3.0.0 alongside with this feature? If so, it might be a good idea to collect the breaking changes in either this issue or create one explicitly dedicated for it. That being said, I dislike the idea of overloading the existing methods to accept doubles. It comes with a number of problems, some of which you have already mentioned. However, In my opinion it would lead to a big mess that would be difficult to maintain. Especially if you use your own internal API you could run into some funny scenarios. Lets consider this: In this MRE, it is not intuitively clear (at least to me), what the result would be. Of course, we have the rule "first come, first serve here, which tells us the first two methods will be used, but that still is very, very unintuitive if you have an even slightly more complicated example or the methods not being directly on top of one another. I am aware that the methods would essentially be the same in 90% of the cases, but even having the possibility for a scenario like this makes me uneasy. Therefore I'd also advocate for a "quick and painless" option, which would be to target the hard switch from float to double in Version 3.0.0. If you are uncomfortable with forcing other peoples hand, you could offer that the 2.x branch is maintained for security updates (if that is even applicable here), and future feature development will permanently be moved to 3.x. I am aware that this is even more maintenance work for a small one-man-army, but if you need help in managing the repository, I can offer my assistance. I will likely use JglTF for the next few years, so I am "stuck" with you anyways😉 I would also be willing to test out the migration, so if you have a branch that you deem "test-ready", give me a message and I'll see how difficult the migration would be for me. As usual, your thoroughness and thoughtfulness is greatly appreciated. Keep it up! |
|
I'm also leaning towads the option to accept that one breaking change, and having a "clean" state afterwards.
There will likely be some other breaking changes. For example, for the support of extensions via #109 . (I hope that I can create a PR soon, but the issue already contains some drafts documenting the progress). But... these breaking changes for the extension support will hopefully mostly be "internal", namely for implementation os the Whether there will be further breaking changes for plain users (be it in the context of extension support, or elsewhere) is not yet clear. But I agree that it could make sense to summarize that in an own issue. I already considered to open an issue for this `float->double´ transition, because this is a breaking change, and a pull request is likely to not draw as much attention as a dedicated issue. But a general issue where the changes for 3.0.0 are summarized may make even more sense. About The example that you gave is certainly a "deeper" level showing where this could cause trouble. I'd argue that if someone offers two methods with the same name that only differ by whether they accept a
I have about 60 repositories on GitHub. I may have received ... about 50 security vulnerability alerts here. It's Jackson. It's always Jackson. (Usually not critical)
The state of this PR should already be ready for such a test. And I already tried it out locally. You can probably imagine that I have a few thousand lines of code that "use" JglTF in one way or another and that are not here on GitHub. The update was usually painless and purely mechanical: Check out the branch. Click through the list of errors in Eclipse. Change An aside: I really have to sort out a few things here as quickly as possible. Right now, I'm in a "limbo" state: I have updated many of my non-public code parts for "double". But some development (extensions, bugfixes) is still happening on the So what I'll likely do next is:
(No timeline, but ... hopefully soon) Another aside: I really appreciate your feedback here and in the other issues! 👍 |
I just created an issue for tracking this at #131 (And I'll likely close this PR soon, because the changes are now already part of the |
#132 🙂 |
|
q.e.d. |
|
Closing this - the changes of this PR are already contained in the |
This is a response to #125 : That pull request suggested to change the type of the node
translation/rotation/scale/matrixproperties fromfloat[]todouble[]. As discussed in this pull request, this change makes a whole lot of sense. In fact, all the types in the API should probably have beendoubleto begin with (except for things like accessor data, of course).This pull request here is intended as a preview for a possible "JglTF 3.0.0", where all
Float/float/float[]types are changed toDouble/double/double[]where applicable.The underlying
implclasses have been generated withJsonModelGen, using this main class, with the additional lineconfig.setNumberType(Double.class);which causes the generator to treat the JSON
numbertype asdouble/Double.The other changes here are only "follow-up" changes (all in this commit). They do affect many classes, and this will affect many clients, and it will be a breaking change. But basically all the changes have been completely trivial: Lines that previously said
node.setTranslation(new float[] { 1.0f, 2.0f, 3.0f });can now just say
node.setTranslation(new double[] { 1.0f, 2.0f, 3.0f });(removing the
fs is optional).I think that it would make sense to "rip the bandaid off quickly" and apply this change throughout the codebase in one breaking change for a new version
3.0.0. In other libraries and environments like JavaScript, JSONnumbers are inherently treated as 64bit floating point (akadouble) values. The computational overhead shuld be negligible nowadays. And precision that is once lost can never be compensated for, so it probably makes sense to keep the precision as high as possible.I'm opening this as a DRAFT for now, because it will likely not be merged in this exact form, but probably as part of broader update for version
3.0.0that - if my time allows it - may also contain improved extension support.