option to include min/max limits on the color field for export_sketchfab #4556
Conversation
…rting to sketchfab
|
Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution! |
|
Please fix the indentation. At the moment your code is not valid Python (see https://results.pre-commit.ci/run/github/88776493/1688125829.Xtb3gkPGTg6Woa7ABHKa8Q) |
|
@evanoconnor Thank you for this pull request! I think this is a good addition, and I appreciate you adding it to the code base. The changes suggested are pretty minor I think, and I'm happy to help out if you'd like to get it across the finish line. |
|
ok, I'm a little bit unsure about the error after the last commit [d736bfc]. It worked on my install, but I think I'm missing something about |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| cs = [float(field) for field in cs] | ||
| cs = np.array(cs) | ||
| mi = cs.min() |
There was a problem hiding this comment.
I'm not sure I understand this change. What's the gain here, compared to mi = cs.min() ?
There was a problem hiding this comment.
I'm not 100% (this is just a copy of the max/min treatment for the export_obj subroutine). I'm guessing this gets rid of the units though?
There was a problem hiding this comment.
ah ! if dropping units is the intention here it should probably just be mi = np.asarray(cs).min()
There was a problem hiding this comment.
I'm fairly baffled by this (and export_obj, so this comment isn't directed at you, @evanoconnor :) ). From what i can tell the input cs to this function will always be an array? so cs = [float(field) for field in cs] a few lines above this will iterate over every element and individually cast to float just to convert back to an array afterwards? that seems not great.... am I missing something?
There was a problem hiding this comment.
I believe this is indeed to drop units, and possibly it's to do some conversion away from numpy arrays, because of some business with the exporting, but I cannot recall.
There was a problem hiding this comment.
cs is used a bit further down to construct the color scale from 0 to 1. So I guess this then:
cs = np.asarray(cs)
mi = cs.min()
before deciding on the best way here, what is the general practice for units? should the color_field_max and color_field_max have units at the onset, that would fail here because cs drops the units and mi and ma would retain them.
There was a problem hiding this comment.
yt's interface regarding scalar values with units isn't exactly consistent at the moment. In many places, we expect floats where we could in principle support passing quantities.
I think it's fine that color_field_min and color_field_max only support float values for now. We can always add support for quantities in the future.
Agreed that cs = np.asarray(cs) is the most robust and efficient way to drop units.
|
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
|
@evanoconnor Would you consider adding yourself to the yt paper? https://github.com/yt-project/yt-4.0-paper/#authorship-policy |
|
@matthewturk Thanks, but I don't quite think this warrants authorship, and I don't have the time to help with the paper (plus it seems like it is nearing the end!) I am looking forward to have such a resource though! |
PR Summary
For exporting surfaces to 3D objects there are a few options, for example
export_blenderandexport_sketchfab.export_sketchfabdoes not include the ability to limit the range of the color field (export_blenderdoes). This PR brings that functionality toexport_sketchfabthrough optional arguments.PR Checklist