Support for virtual filesystems through IOSystem #10
Conversation
There was a problem hiding this comment.
Hello Hugo, and thank you for your contribution!
I have two main issues with this before I would consider accepting this, however. Keep in mind this was a quick review, and I may have missed how it works entirely.
- Change iser-passed files back to a URI. This could be negotiated, but URI's typically are more java/kotlin safe as creating an invalid URI will throw an exception for the user, not the library (or, it can be created from a file object. long story short: not strings)
- Use one default importer everywhere (assimp.kt companion object, maybe?)
- Default function arguments: that way, user doesn't have to change code if they don't want to after upgrade
Generally, I do want to remove LWJGL. Clunky solution for now, but, trying to add more base functionality, If you want to remove all of it, please go ahead!
Thank you for taking your time on this, hope to get it pulled soon!
| @@ -191,6 +191,11 @@ constructor() { | |||
| } | |||
| } | |||
|
|
|||
| var ioHandler: IOSystem | |||
There was a problem hiding this comment.
This is the way the original assimp does it though
http://assimp.sourceforge.net/lib_html/class_assimp_1_1_importer.html#a1161f46318af18bb86dfe0fc3edea4df
Is there some Kotlin trickery I can use to mirror that into the importers ( probably has to do with var and get() = something something ) ?
There was a problem hiding this comment.
Actually, after playing around myself, you're good on this.
| @@ -33,7 +33,7 @@ abstract class BaseImporter { | |||
| * time to examine the contents of the file to be loaded for magic bytes, keywords, etc to be able to load files | |||
| * with unknown/not existent file extensions. | |||
| * @return true if the class can read this file, false if not. */ | |||
| abstract fun canRead(file: URI, checkSig: Boolean): Boolean | |||
| abstract fun canRead(file: String, pIOHandler: IOSystem, checkSig: Boolean): Boolean | |||
There was a problem hiding this comment.
Any user functions should have this same idea applied
| import java.io.* | ||
|
|
||
| class DefaultIOSystem : IOSystem{ | ||
| override fun Exists(pFile: String) = File(pFile).exists() |
There was a problem hiding this comment.
Lowercase function names
src/main/kotlin/assimp/IOSystem.kt
Outdated
|
|
||
| fun Close(ioStream: IOStream) = Unit //unused ? | ||
|
|
||
| fun getOsSeperator() = "/" |
There was a problem hiding this comment.
Separator: java.io.File.separator
| @@ -191,6 +191,11 @@ constructor() { | |||
| } | |||
| } | |||
|
|
|||
| var ioHandler: IOSystem | |||
There was a problem hiding this comment.
This is the way the original assimp does it though
http://assimp.sourceforge.net/lib_html/class_assimp_1_1_importer.html#a1161f46318af18bb86dfe0fc3edea4df
Is there some Kotlin trickery I can use to mirror that into the importers ( probably has to do with var and get() = something something ) ?
Oh don't worry, everyone has to start from somewhere sooner or later :) So, did you like it? Kotlin is generally one of the most loved language in statistics and its adoption is rising strong..
Glad to read that, because that's exactly my intention: having a full implementation shaped somehow for the jvm platform and the jvm developing environment.
Yep, that is (was) one of the points on the todo list. One further point is to move to kotlin-multiplatform (more here and here). This would easier the co-existence of desktop and Android (and iOS)
We'll work on that step by step, that's fine for a transition process
That's great, thanks
I'm glad to read that too, I wanted to (have an option to) destinate as much as possible boilerplate code to the lib and leave the application as minimal as possible
At the moment no, but if it's causing conflict we can take in account it. As far as I recall, it's just a pure convenient use at the moment, nothing critical, except the gli dependency, for the texture loading (it has to do with native memory management) Would it make sense (and would you available) to work on a brand new branch for all of those changes? Thanks again for the huge push :) |
|
After testing myself, I'd approve after the function name changes and the correct system separator are applied. This is a good piece of work, thank you for your time! Side note: doesn't matter you're new to kotlin. My first pull to kotlin-graphics was in imgui, and I only used notepad++. As long as you're helping, we're completely okay with it. |
|
Thanks for the encouragement! I'm always committed to too many projects, so I can't say I've looked in depth at Kotlin, it's mostly to unlock the situation on my project end. It does give me a chance to try out that free copy of IntelliJ I get as a student and toy with new stuff, so that's always fun. Kotlin is a very powerful beast indeed, but I'm not so fond of IntelliJ. It might be the familiarity with Eclipse, but it's too insistent on holding my hand and at the same time lacks stuff like pop-up javadoc and suggestions. Well, anyway, here's what the 3 latest commits are about The first one fixes issues with (again) the Collada importer, it would not give me my bones ( and we all know how being boneless suck ), i've compared the code with the cpp version and the output with what lwjgl's assimp bindings give me and tracked the problem down, at least with my sample file. As I'm integrating this into my engine while refactoring my mesh/animations subsystems, I get to fix any bug in the way. Lucky me I guess Second one changes the library to only use the slfj-simple backend when actually running tests. I had changed it to use the API only in a previous commit, and that had the side effect of making all test succed, even when they should fail ( the fbx tests fail in the master branch ) Third change fixes the uppercase methods in IOSystem, adds a bunch of default parameters and fixes the folder seperator. I've experimented with reverting back to using URIs, but that doesn't mesh well at all with my virtual filesystem, as URIs seem to insist on checking they actually reference files on the disk ( and can't find any since these are indeed not files on the disk ) and don't give me an easy way to go back into a parent folder then into another file. The original assimp uses just plain strings, and it seems just not worth the headache to me. |
|
Sounds good. Looks good to pull, not sure what's with the FBX failiure |
|
Hi guys, I just updated the Do you have any tips where it may come from? Or how may I fix it? Ps: @Hugobros3 I sent you an invite as collaborator, so in the future you may directly push |
|
Hey thanks for the invite! Not sure where the issue comes from, it's weird indeed. Thinking about it, I think it's something else. If you're running the tests from IntelliJ and bypassing the gradle script, you're running them from a different working directory ( the root of the project folder likely ) and the test cases files are in src/test/resources/. I had to do this since I've moved away from URL()s and directly using File()s under an abstraction layer. A quick fix could be to modify the DefaultIOSystem to account for that and remove the above code snipet from the build.gradle. I just always run tests from the command line so it never occurred to me. I should also mention I've made my own branch where I've implemented further changes to the code in order to integrate it with my own game engine. https://github.com/Hugobros3/assimp/tree/no-lwjgl The LWJGL dependency turned out to be a major pain so I made the cuts necessary to get rid of it. I only really need a few importers working (.dae, .obj for now) and have no need for loading the embedded textures. |
How would you do that? This means
Uhm, how would you debug them? What's your usual setup?
I guess this could also maybe be interesting for other users, or? Would you consider having your branch here and taking care of it? |
It really just needs to be able to see files on the classpath (ie getClass().getRessource() ) so the test would work... I think
Eclipse :p I never really debug tests directly, and when I do it's with good ol' sysouts. Yeah...
It's really just edits to make it work, or work better with my codebase. Some stuff would be interesting to backport, like bugfixes or removing the LWJGL dependency, if I find the time to fix the broken stuff that depended on it. Do you have a discord/slack or something by any chance ? |
|
After some searches and analyses, I think we should migrate to
Related: I'd start playing with that next week, but in the meanwhile, if you have some, I'd like to have some feedback from you guys about it
Don't do that, save yourself tons of time. Just take a look what debug in Idea looks like
Sure, I'm on the kotlin slack the whole time. Or if it doenst work for you, I may come on discord |
|
One issue: some VFS's don't support path
…On Sat, Apr 14, 2018, 02:01 Giuseppe Barbieri ***@***.***> wrote:
After some searches and analyses, I think we should migrate to Path for a
couple of reasons, among:
- it's modern and supposed to replace File
- it can do everything File can, but better and something more
Related:
https://stackoverflow.com/a/40725390/1047713
http://www.oracle.com/technetwork/articles/javase/nio-139333.html
https://docs.oracle.com/javase/tutorial/essential/io/legacy.html
I'd start playing with that next week, but in the meanwhile, if you have
some, I'd like to have some feedback from you guys about it
Eclipse :p I never really debug tests directly, and when I do it's with
good ol' sysouts. Yeah...
Don't do that, save yourself tons of time. Just take a look what debug in
Idea looks like
<https://d3nmt5vlzunoa1.cloudfront.net/idea/files/2015/12/debug_slots.png>
Do you have a discord/slack or something by any chance ?
Sure, I'm on the kotlin slack <https://kotlinlang.slack.com> the whole
time. Or if it doenst work for you, I may come on discord
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#10 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOafQLhAS4GJArnbjsyJw7C2Y6oa90HXks5toZ60gaJpZM4S-5qv>
.
|
|
The way the refactor works as of now is using Strings for filenames, and only DefaultIOSystem actually uses files ( at least it should once all the importers are migrated to use an IOSystem ), do you mean using Paths in DefaultIOSystem implementation or in the end-user API ? Because again, with the later option you'd be enforcing applications to expose whatever flavour of VFS they have with a Path implementation, a FileSystem implementation and so on. I already have my own abstractions in Chunk Stories, so meh. Again the vanilla cpp assimp doesn't bother with any of this and just go with std::string everywhere, and I appreciate the simplicity. If you just mean using Paths instead of Files in DefaultIOSystem I'm totally with you on the other hand. |
Uh, for example? I couldn't find anything
Yeah, the first one |
I was thinking of VFSes in a broader sense: anything that has some abstract idea of filenames that can spout bytes, including most game engines resources/mod managers. But with your second reply I understand that you speak about the ones on the actual filesystems, and for those we can totally use Paths |
|
So, to sum up, I basically simply changed Also, md3 and fbx tests fixed, a couple of dependencies bumping and released |
|
@Hugobros3 these guys need a lwjgl-less assimp version (or better without any native dependency) Would you be so kind to push it as a branch? |
First, a disclaimer: I'm a complete Kotlin noob and I'm learning as I go.
I have a Java project that requires a good way to load in models, and LWJGL's assimp bindings are kind of bare, and any tweaking to assimp itself would require doing C++ and a custom build of LWJGL, so I can't be bothered. This seems like a better option, but there was a lack of the IOSystem interface from the original library that allows for custom filesystems ( implements Open/Close read/seek/write functionality ).
I've went ahead and added that, along with a default implementation that just uses regular File[s]. I've also modified a few importers to make use of them, but haven't yet implemented those who need random access instead of an InputStream/BufferedReader. Tests all run fine, even whose who weren't working in the master revision for some reason, even though I didn't touch that.
I've also fixed two issues with the Collada loader that would not parse files with vertex weights ( or at least, wouldn't parse mine ). The first issue was related to an ArrayList being indexed but not initialized with any capacity, I fixed it by changing it into a LongArray. The second one was a -1 access when disabling ASSIMP_LOAD_TEXTURES ( a feature the cpp version doesn't have and I really need )
Finally, I must ask whether it is planned to drop the requirement from having LWJGL in this library's path. It's potentially causing conflicts with other version of LWJGL, and generally unwanted (the original assimp has no such dependencies). It's something that would make this library much more clean, at least in my eyes.