kotlin-graphics / assimp

JVM Open Asset Import Library (Assimp)
Other
95 stars 28 forks source link

Support for virtual filesystems through IOSystem #10

Closed Hugobros3 closed 6 years ago

Hugobros3 commented 6 years ago

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.

elect86 commented 6 years ago

First, a disclaimer: I'm a complete Kotlin noob and I'm learning as I go.

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..

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.

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.

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 ).

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)

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.

We'll work on that step by step, that's fine for a transition process

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.

That's great, thanks

The second one was a -1 access when disabling ASSIMP_LOAD_TEXTURES ( a feature the cpp version doesn't have and I really need )

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

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.

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 :)

Sylvyrfysh commented 6 years ago

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.

Hugobros3 commented 6 years ago

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.

Sylvyrfysh commented 6 years ago

Sounds good. Looks good to pull, not sure what's with the FBX failiure

elect86 commented 6 years ago

Hi guys,

I just updated the master and almost all the tests are failing because the file to read cant be found. But the travis build is fine (except the fbx tests).

Do you have any tips where it may come from? Or how may I fix it?

https://imgur.com/a/dRLMI

Ps: @Hugobros3 I sent you an invite as collaborator, so in the future you may directly push

Hugobros3 commented 6 years ago

Hey thanks for the invite!

Not sure where the issue comes from, it's weird indeed. Maybe some platform-specific issue with folder separators ? I worked on Windows 8.1 using IntelliJ and Git Bash, maybe that allowed some more flexibility in handling Unix-style '/' separators that Windows 7 wouldn't ?

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/.

test {
    workingDir = "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.

elect86 commented 6 years ago

A quick fix could be to modify the DefaultIOSystem to account for that

How would you do that? This means DefaultIOSystem shall be aware where it's running on.. or?

I just always run tests from the command line so it never occurred to me.

Uhm, how would you debug them? What's your usual setup?

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.

I guess this could also maybe be interesting for other users, or? Would you consider having your branch here and taking care of it?

Hugobros3 commented 6 years ago

How would you do that? This means DefaultIOSystem shall be aware where it's running on.. or?

It really just needs to be able to see files on the classpath (ie getClass().getRessource() ) so the test would work... I think

Uhm, how would you debug them? What's your usual setup?

Eclipse :p I never really debug tests directly, and when I do it's with good ol' sysouts. Yeah...

Would you consider having your branch here and taking care of it?

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 ?

elect86 commented 6 years ago

After some searches and analyses, I think we should migrate to Path for a couple of reasons, among:

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

Do you have a discord/slack or something by any chance ?

Sure, I'm on the kotlin slack the whole time. Or if it doenst work for you, I may come on discord

Sylvyrfysh commented 6 years ago

One issue: some VFS's don't support path

On Sat, Apr 14, 2018, 02:01 Giuseppe Barbieri notifications@github.com 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 https://github.com/kotlin-graphics/assimp/pull/10#issuecomment-381308941, or mute the thread https://github.com/notifications/unsubscribe-auth/AOafQLhAS4GJArnbjsyJw7C2Y6oa90HXks5toZ60gaJpZM4S-5qv .

Hugobros3 commented 6 years ago

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.

elect86 commented 6 years ago

One issue: some VFS's don't support path

Uh, for example? I couldn't find anything

do you mean using Paths in DefaultIOSystem implementation or in the end-user API ?

Yeah, the first one

Hugobros3 commented 6 years ago

Uh, for example? I couldn't find anything

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

elect86 commented 6 years ago

So, to sum up, I basically simply changed path type to Path

Also, md3 and fbx tests fixed, a couple of dependencies bumping and released

elect86 commented 6 years ago

@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?