Sean (Feb 25 2022 at 22:14): Sean (Feb 25 2022 at 22:14):@starseeker FYI, the inclusion of nearly all tinygltf was intentional -- I messaged on teams, but the gist is that was a working setup with sample data and examples that this semester's students were given as a starting point. plus, I think it'll simply be less effort to manage as updates to tinygltf are expected possibly repeatedly over the next couple months while they work. I don't know their plans for that code yet, we may not even keep it as it's a working reference.
starseeker (Feb 25 2022 at 22:15):Can we put it in src/other/ext? It's pretty messy for the main tree
Sean (Feb 25 2022 at 22:15):I mean, nothing else uses it and probably never will...
Sean (Feb 25 2022 at 22:16):that's the pattern we used elsewhere when something is only used once
Sean (Feb 25 2022 at 22:16):fyi I was also thinking to apply the same dist inclusion pattern I settled on there as it solves the problem of quoting files and needing to know the var name
starseeker (Feb 25 2022 at 22:17):I hate cluttering up the main tree that way... when I do that elsewhere, I usually make an effort to strip things down to a minimal core
starseeker (Feb 25 2022 at 22:17):I'll put it back for the student efforts, but I'm not crazy about it.
starseeker (Feb 25 2022 at 22:20):Give me a minute so I can try and do it without breaking the regress checks.
Sean (Feb 25 2022 at 22:21):There's two concerns, there's clutter in the tree and there's maintainability across updates too. I'd think there's considerably less effort overall the less we modify 3rd party deps, no matter where they are. openNURBS being a great example of edits essentially halting maintenance and development.
Sean (Feb 25 2022 at 22:22):all to save a few bytes or reduce a directory listing a few lines, which I don't think impose a technical problem (presently at least), more an aesthetic one
starseeker (Feb 25 2022 at 22:25):To me it's more than just aesthetics - when something goes into the main tree, I want to be able to understand easily why its there. As an ideal, I'd like to be able to look at every file in the source tree that's not in src/other or misc/tools and be able to identify why it's there from a BRL-CAD perspective.
Sean (Feb 25 2022 at 22:25):What makes it feel like clutter? Or what's the consequence/effect? we have another example in src/libged/bullet where even the bullet source files are hundreds of lines, and they're necessary for the feature, but don't appear to appreciably affect anything by being in the tree
Sean (Feb 25 2022 at 22:26):(other than the same consequence as other libs -- since it's not a drop-in, I can't update and test the latest bullet without considerable effort)
starseeker (Feb 25 2022 at 22:27):bullet should be in src/other/ext - it was my own technical inability to get it working successfully (on Windows, IIRC) any other way that landed it there.
starseeker (Feb 25 2022 at 22:28):Anyway, I'll get the gltf stuff back for the students.
Sean (Feb 25 2022 at 22:28):starseeker said:
To me it's more than just aesthetics - when something goes into the main tree, I want to be able to understand easily why its there.
but isn't saying "src/conv/gltf/tinygltf is there because that's tinygltf and gltf-g uses tinygltf"? at least it is in my mind, though I could see a case for something like src/conv/gltf/other/tinygltf or src/conv/gltf/ext/tinygltf to really indicate in the path itself that it's a local dependency, and graduate to src/other when it's multiply used.
starseeker (Feb 25 2022 at 22:30):The latter would be better in that the repository checking tools could be taught more easily to treat those patterns differently
Sean (Feb 25 2022 at 22:30):I mean I'd also be cool putting ALL external deps in src/other, but that seems like additional work for little benefit, especially for header-only stuff or single file situations. hard to justify.
starseeker (Feb 25 2022 at 22:32):I guess I could see a convention where anything inside an */ext/ directory is handled as external source code, and it wouldn't have to necessarily be in src/other
starseeker (Feb 25 2022 at 22:33):But I'd want some convention like that, which makes it clear that our standards for organization, build flags, etc. don't need to apply.
Sean (Feb 25 2022 at 22:34):we should figure something out, as I see that really being an issue for GCV.
Sean (Feb 25 2022 at 22:34):(which notionally is where gltf will land by the time summer is here)
Sean (Feb 25 2022 at 22:35):I was going to try and move one of the other converters to get a feel for the effort needed.
starseeker (Feb 25 2022 at 22:35):Agreed. I'll go ahead and put it back for now, once I figure out how to do so without the github CI checks breaking, but I would like to have some definite scheme in place.
Sean (Feb 25 2022 at 22:36):okay, definitely appreciated for this -- I literally just showcased the folder to them yesterday and told them to git pull and explore :)
starseeker (Feb 25 2022 at 22:36):Ow. Alright, it'll probably take a hour or two - keep an eye out for the push
Sean (Feb 25 2022 at 22:38):they have a huge mountain of concepts they're climbing, so trying to make things as absolutely simple as possible. only thing I didn't do for the converter team was drop in the Khronos gltf test cases they're going to conform against, but that'll need to go somewhere too eventually.
Sean (Feb 25 2022 at 22:40):it's technically test infrastructure, so probably something like src/gcv/tests/gltf
starseeker (Feb 25 2022 at 22:42):If we can come up with a few simple directory naming patterns we can put in the repository regression tester and similar tools so they know to treat the contents of those dirs differently, and then use those reliably, that would probably work.
starseeker (Feb 25 2022 at 22:47):@Sean were you deliberately leaving some whitespace issues for them to find and fix?
Sean (Feb 25 2022 at 22:49):I like ext, short and sweet, would parallel src/other/ext and could be managed similarly. you have a nice simple setup there, so lets run with it.
Sean (Feb 25 2022 at 22:51):starseeker said:
Sean were you deliberately leaving some whitespace issues for them to find and fix?
Not find and fix, no - the instruction was simply that it's working reference code. It "works" in the sense that it compiles and they can directly see how it was derived from the loader example in tinygltf, but it doesn't raytrace and isn't structured.
Sean (Feb 25 2022 at 22:52):their task is to make a gcv plugin, so src/conv/gltf will probably get deleted when they get theirs working
starseeker (Feb 25 2022 at 22:52):OK, just making sure I wasn't blowing away a test case ;-)
Sean (Feb 25 2022 at 22:53):that whole dir was a compilable reference for them, they may choose to start with it as a base -- and if they do, then we'd have a talk about the code conventions being wrong. I had in mind to pretty-print format the code, but was keeping it as-is for now just so they can line-compare with where it came from
Sean (Feb 25 2022 at 22:54):but yeah, I don't think formatting will make or break their understanding
Sean (Feb 25 2022 at 22:55):half of them use VS Code which does NOT like our style by default.. indentation is all screwed up
starseeker (Feb 25 2022 at 22:55):We never did get around to the astyle reformat, did we
Sean (Feb 25 2022 at 22:55):nope, with good reason -- I think about that often :)
Sean (Feb 25 2022 at 22:56):would invalidate every outstanding patch and code we have pretty hard... I'd at least want to try and merge as much as possible first
starseeker (Feb 25 2022 at 22:57):OK, I (more or less) put it back - I'm not sure what to do about the static analyzer though, other than bump the count back up
starseeker (Feb 25 2022 at 22:59):I got a few in the main converter file, but IIRC some of the complaints are from the stb headers.
starseeker (Feb 25 2022 at 23:03):I'll know in a bit... re-running now
Sean (Feb 25 2022 at 23:08):what's it complaining about?
Sean (Feb 25 2022 at 23:09):way to exclude it perhaps since it's not production?
starseeker (Feb 25 2022 at 23:10):I don't really know of a good way to tell clang to ignore code coming in from particular headers, unfortunately
starseeker (Feb 25 2022 at 23:11):Dead store Dead assignment conv/gltf/tinygltf/stb_image.h stbi__tga_load 5708 1
Dead store Dead increment conv/gltf/tinygltf/stb_image.h stbi__high_bit 5079 1
Dead store Dead initialization conv/gltf/tinygltf/stb_image.h stbi__tga_load 5531 1
Dead store Dead initialization conv/gltf/tinygltf/stb_image.h stbi__parse_png_file 4890 1
Dead store Dead initialization conv/gltf/tinygltf/stb_image.h stbi__tga_load 5530 1
Dead store Dead nested assignment conv/gltf/tinygltf/stb_image.h stbi__tga_load 5709 1
Dead store Dead nested assignment conv/gltf/tinygltf/stb_image.h stbi__zexpand 4015 1
Dead store Dead nested assignment conv/gltf/tinygltf/stb_image.h stbi__tga_load 5708 1
Dead store Dead nested assignment conv/gltf/tinygltf/stb_image.h stbi__tga_load 5709 1
Dead store Dead nested assignment conv/gltf/tinygltf/stb_image.h stbi__tga_load 5708 1
Unix API Allocator sizeof operand mismatch conv/gltf/tinygltf/stb_image_write.h stbi_zlib_compress 875 1
starseeker (Feb 25 2022 at 23:13):Can be cleared easily enough - I already did so in fact - but then we're back to not being vanilla
starseeker (Feb 25 2022 at 23:13):If this turns out to be production code I'm OK with that, but since we're not sure this is the answer...
starseeker (Feb 25 2022 at 23:13):I'll just bump the expected count for now.
Sean (Feb 25 2022 at 23:14):Yeah, I was giving them this week to look it over, and then recommend they implement what they want to do from scratch (using it as a reference or whatever, but do so cleanly).
Sean (Feb 25 2022 at 23:15):could exclude src/conv/gltf/gltf-g.c ? just leave a comment so we can revisit in 3 months
Sean (Feb 25 2022 at 23:16):it's kind of funny.. stb_image.h is a header-only library that implements a whole slew of image file formats, used by tinygltf which is also header only
starseeker (Feb 25 2022 at 23:16):I'm not really set up to do that any more, unfortunately - I simplified the analyzer script when I got the bug count down, so I'm no longer calling out very many individual targets.
Sean (Feb 25 2022 at 23:16):okay
starseeker (Feb 25 2022 at 23:16):Bumping the count is the simplest thing for now, and if we use 'em for real we'll have to try and clean 'em up
starseeker (Feb 25 2022 at 23:17):I think they trigger a few Coverity issues as well, for that matter
starseeker (Feb 25 2022 at 23:20):(I didn't shift it into an ext subdir yet, so your instructions should be good again.)
starseeker (Feb 25 2022 at 23:26):You know, with the new ext build setup I should probably revisit bullet - if I'm using their build system as-is I might not have the problems we had before.
Last updated: Oct 09 2024 at 00:44 UTC