read file from memory #16

Wasabi375 · 2018-09-03T17:16:05Z

I'm still working on it but I thought I might as well create the pr now, so that this is visible.
There are still a few tests failing. I'm not sure if this is due to the fact that some tests fail on master or that some of the Importers are still trying to access files on the drive directly as I have not yet looked at those tests.

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

some todos about importers not working from memory Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Sylvyrfysh · 2018-09-03T17:25:12Z

Why does XFileImporter check if the file exists before opening, while BlenderLoader does not? I see blender is disabled, but would still like the coding to be consistent. My personal opinon on this is that we should not be checking if the file exists since the open function appears to check if the file exists itself.

Also, the exists function in DefaultIOStream compated to Files.exists(path) should be addressed. Both exist in the code and it leads to a little more fragmentation I feel could be unified.

Sylvyrfysh

I would remove the checks if files exist myself as open will throw a IOException otherwise, and a stacktrace will be sufficient to see which model opening failed.

src/main/kotlin/assimp/format/fbx/fbxImporter.kt

Sylvyrfysh · 2018-09-03T17:26:08Z

src/main/kotlin/assimp/format/md2/Md2Loader.kt

-
        // Check whether we can read from the file
-        if (!file.canRead())
+        if (!ioSystem.exists(file))


but we again check here

src/main/kotlin/assimp/DefaultIOSystem.kt

Sylvyrfysh · 2018-09-03T17:28:03Z

src/main/kotlin/assimp/MemoryIOWrapper.kt

+		// with positions that match each other or not?
+		// If I move duplicate back here I need to also adjust the byte order here
+		// Then I no longer need to change it in MemoryIOSystem.open
+		override fun readBytes(): ByteBuffer = buffer


I would call duplicate here, then remove all other duplicates in the code while making sure the internal buffer is not used

I think you are right. This way there should also be only this one place where we need to call duplicate which means that we only have to call order(ByteOrder.nativeOrder()) once. For some reason duplicate resets the byteOrder to BIG_ENDIAN.

src/main/kotlin/assimp/format/md3/Md3Loader.kt

Sylvyrfysh · 2018-09-03T17:29:59Z

src/main/kotlin/assimp/format/obj/ObjFileImporter.kt


        // Read file into memory
        this.file = file//File(file)
        if (!ioSystem.exists(file)) throw IOException("Failed to open file $file.")


not your change, but we check if exists here. Change?

Sylvyrfysh · 2018-09-03T17:30:15Z

src/main/kotlin/assimp/format/ply/PlyLoader.kt

-
        // Check whether we can read from the file
-        if (!file.canRead()) throw IOException("Failed to open PLY file $file.")
+        if (!ioSystem.exists(file)) throw IOException("Failed to open PLY file $file.")


checking if exists

Sylvyrfysh · 2018-09-03T17:30:52Z

src/main/kotlin/assimp/format/stl/STLLoader.kt


        // Check whether we can read from the file
-        if (!file.canRead()) throw IOException("Failed to open STL file $pFile.")
+        if (!ioSystem.exists(file)) throw IOException("Failed to open STL file $file.")


checking if file exists

Wasabi375 · 2018-09-03T21:25:41Z

I agree with you I think. I did not yet take the time to make sure all the different importers are consistent in the way the do things. I noticed of cause that they are. For some reason one of them is using a pointer to a ByteArray instead of a Buffer for example.
I was planning on going over some of it. AFAIK they follow the original C code closely which might explain some of it. I'll see what I can do there.
I also like your idea of removing the existence checks before calling open. They are as you pointed out unnecessary. I just added them because they where there already just using the "old" system.

The places where it's still using File(filename) I just have not yet changed. The reason is that some of the file formats expect multiple files or at least sometimes do (I think md3, md5 and obj). I either need to change MemoryIOSystem so it can somehow contain multiple files or create another class maybe MemoryMultiFileIOSystem. I tend towards the first solution.

Something else to consider is the filename for memory files, especially in memoryIOSystems with multiple files. They can't all be called AI_MEMORYIO_MAGIC_FILENAME and so far I don't see any reason why any of them needs to be called like this. As far as I can tell this name will not work with the multi-file formats anyways as they need the filename to find the other files.

ObjImporter now loads multi file objects Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

src/main/kotlin/assimp/format/obj/ObjFileParser.kt

src/test/kotlin/assimp/obj/obj.kt

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

…with a different test name) Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

…versions Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Wasabi375 · 2018-09-06T03:19:44Z

src/main/kotlin/assimp/format/md3/Md3Loader.kt

-                Q3Shader.loadShader(fill, "$path../../../scripts/$filename.shader")
+            // TODO read from memory: how do we resolve ../../..
+            if (!Q3Shader.loadShader(fill, "$path../../../scripts/$modelFile.shader", ioSystem))
+                Q3Shader.loadShader(fill, "$path../../../scripts/$filename.shader", ioSystem)


I'm not really sure what to do here. I could implement a fully functioning path system but I'm not sure if that is not a bit overkill. The other option would be, that if you want to load relative paths like this from memory that you need to specify those in the name. So you actually name a file "somePath/../../../scripts/moduleName.shader".
This would work right now. Don't think it's a good solution though...

If we don't find a solution now, we can postpone the decision until someone needs this and find together a solution

Well I see 3 solutions:

leave it as it is, there is a simple workaround I used in the test

modify the ioSystem so that it resolves ".." in a way that it removes the last directory

change the ioSystem so it uses java.nio.file.Path instead of Strings to represent a file. That way resolving paths would be trivial, but implementing Path and FileSystem/FileSystemProvider takes a lot of effort and I don't think it's worth it.

I think for now we can leave it and maybe implement option 2 later.
I created an issue for it: #17

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Wasabi375 · 2018-09-06T05:42:32Z

So long as I have not missed anything, I think this should be all 😉 Would be nice if you guys could look over it again.
Also I think this might be a good point to push a new release. The current one is broken after all.

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

elect86 · 2018-09-06T06:49:21Z

For me is fine, as soon as @Sylvyrfysh gives me the green light I can publish it

Wasabi375 · 2018-09-06T07:07:32Z

src/main/kotlin/assimp/format/obj/ObjFileImporter.kt

 }


+// TODO this is pretty much a copy past from gli.read(...) and should be added there


@elect86 You are working on gli right? Can you add a readFromMemory there or should I just sent a PR there myself?

I sent you an invite for the organization, so that you can automatically work on that without me having to invite for each single repo (I did the same for @Sylvyrfysh)

Sylvyrfysh · 2018-09-06T14:13:37Z

Looks good!

elect86 · 2018-09-06T20:03:49Z

I was trying to run the "build" gradle task but the tests seems not perceived

Also, trying to run a single test, such as assbin, fails with the following

Does it work on your machines, guys?

Edit: looking more carefully the html reports shows most of the tests failing because of


java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/Users/elect/IdeaProjects/assimp/build/resources/test/models/X//BCN_Epileptic.X
	at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
	at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
	at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
	at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
	at java.base/java.nio.file.Paths.get(Paths.java:84)
	at assimp.DefaultIOSystem.open(DefaultIOSystem.kt:14)
	at assimp.format.assbin.AssbinLoader.canRead(AssbinLoader.kt:23)
	at assimp.Importer.readFile(Importer.kt:281)
	at assimp.Importer.readFile(Importer.kt:237)
	at assimp.AnchorKt.testFile(anchor.kt:45)
	at assimp.AnchorKt.testFile(anchor.kt:33)
	at assimp.AnchorKt.testFile$default(anchor.kt:32)
	at X.BCN_Epileptic$1.invoke(BCN_Epileptic.kt:14)
	at X.BCN_Epileptic$1.invoke(BCN_Epileptic.kt:8)
	at io.kotlintest.runner.jvm.TestCaseExecutor$executeTestSet$1.run(TestCaseExecutor.kt:94)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.base/java.lang.Thread.run(Thread.java:844)

Wasabi375 · 2018-09-06T21:32:29Z

Are you running own windows? It's the only platform I have not tested. All tests run fine on my mac and the travis build is on linux I think. I'll load the project up on my windows computer tomorrow and see if I get the same problem.

Wasabi375 · 2018-09-06T21:36:46Z

Also if you look at the path it's trying to load, it seems to be invalid.
It starts with "/C:/". But there should not be a / at the start. I guess it is trying to resolve it as a relative path which is not allowed to contain colons. I have no idea though why it would create this path.

Sylvyrfysh · 2018-09-07T03:42:29Z

Confirmed. Will try to fix

…

On Thu, Sep 6, 2018, 16:36 Burkhard Mittelbach ***@***.***> wrote: Also if you look at the path it's trying to load, it seems to be invalid. It starts with "/C:/". But there should not be a / at the start. I guess it is trying to resolve it as a relative path which is not allowed to contain colons. I have no idea though why it would create this path. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOafQJ8gPZW69_W0IPOUwzJG30yKU9_9ks5uYZVvgaJpZM4WX5hZ> .

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Wasabi375 · 2018-09-07T05:49:05Z

I can't reproduce the bug @elect86 described. I just tested the project on my windows pc as well. I found another small bug though. I hate that windows is using \ instead of / as a separator.....

Sylvyrfysh · 2018-09-07T14:55:55Z

Whatever change you made, worked. Windows works again for me

Wasabi375 · 2018-09-07T17:05:21Z

Strange, my change should only effect MD3 loading.

elect86 · 2018-09-07T20:10:56Z

I use both window and linux, although I was on windows 10 x64 when I tested that.

Problem persist, I'll try to debug. Windows is indeed more tedious in this regard..

However I wonder why the test result interface changed in Idea, I recall in the past all the test, passed and not, were listed, now no more. Do you have any tips about that?

elect86 · 2018-09-07T20:42:49Z

So, I managed to fix it on my machine. I'd like now a feedback from you :)

Ps: I notice that I can have the test interface I meant if I run a test singularly

https://imgur.com/g7Ej6F6

Sylvyrfysh · 2018-09-07T20:48:33Z

thoroughly off topic but still relevant: should we create a slack or something so we can communicate easier on this

Sylvyrfysh · 2018-09-07T20:58:16Z

did the windows fix get merged here?

elect86 · 2018-09-07T21:02:11Z

Our traffic is quite low in average, therefore I'd suggest to use lwjgl or kotlin slack

Actually I messed up, I thought to pull his modifications, but I was working on the master, now I reverted my mistaken push.. in this regards, how can I pull a push request?

Ps: thanks @Wasabi375 for your push :)

Edit: it seems there is still no viable gui option, https://stackoverflow.com/questions/40217494/how-to-review-a-pull-request-in-intellij-idea

Sylvyrfysh · 2018-09-08T17:13:44Z

Do you still want to un-pull?

elect86 · 2018-09-08T17:40:19Z

No, it's fine, thanks

Followup to PR #16 (read from memory)

Wasabi375 added 8 commits August 31, 2018 20:02

some todos

affb662

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

enabled read from memory tests

0415bfc

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

fixed readFromMem for FBX

219f838

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

fixed readFromMem for Direct X Importer

bb201d6

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

added IOStream size, not yet used everywhere

8416b26

some todos about importers not working from memory Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

cleaning up code adding more todos *sigh* 😉

8d476a6

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Refactored ByteBuffer creation from IOStream in importers

d58d67e

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Fixed byte order bug in read from memory

d8d1101

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Sylvyrfysh requested changes Sep 3, 2018

View reviewed changes

Wasabi375 added 2 commits September 5, 2018 23:55

MemoryIOSystem now supports multiple files

ba4580d

ObjImporter now loads multi file objects Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

removed unnecessary file.exists checks

9b6c20d

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Sylvyrfysh reviewed Sep 5, 2018

View reviewed changes

src/main/kotlin/assimp/format/obj/ObjFileParser.kt Show resolved Hide resolved

src/test/kotlin/assimp/obj/obj.kt Show resolved Hide resolved

Sylvyrfysh approved these changes Sep 5, 2018

View reviewed changes

Wasabi375 added 5 commits September 6, 2018 02:16

removed some warnings

0d00c5c

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

removed boblampclean_assbin.kt (file is exact copy of assbin.kt just …

1070491

…with a different test name) Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Fixed bug where URLs with spaces would not be resolved

fef0c5d

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Fixed read byte bug in ByteBufferBackedInputStream. stupid number con…

0dc97b8

…versions Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

MD2 and MD5 is working properly. MD3 is stupid

f7c9b0e

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Wasabi375 commented Sep 6, 2018

View reviewed changes

Wasabi375 added 3 commits September 6, 2018 07:00

fix bug using wrong reference to ioSystem

01c4c02

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

MD3 relative path workaround

a1df627

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

some final cleanups

04f84f0

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

final final cleanup

56fe528

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

Wasabi375 requested a review from Sylvyrfysh September 6, 2018 05:54

Wasabi375 commented Sep 6, 2018

View reviewed changes

Sylvyrfysh approved these changes Sep 6, 2018

View reviewed changes

fixed osSeperator bug on windows

31e2f45

Signed-off-by: Burkhard Mittelbach <wasabi37a@googlemail.com>

elect86 merged commit 40bea13 into kotlin-graphics:master Sep 7, 2018

elect86 added a commit that referenced this pull request Sep 23, 2018

Merge pull request #18 from Wasabi375/master

5276236

Followup to PR #16 (read from memory)

		}


		// TODO this is pretty much a copy past from gli.read(...) and should be added there

read file from memory #16

read file from memory #16

Uh oh!

Conversation

Wasabi375 commented Sep 3, 2018

Uh oh!

Sylvyrfysh commented Sep 3, 2018

Uh oh!

Sylvyrfysh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wasabi375 commented Sep 3, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wasabi375 Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wasabi375 commented Sep 6, 2018

Uh oh!

elect86 commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sylvyrfysh commented Sep 6, 2018

Uh oh!

elect86 commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wasabi375 commented Sep 6, 2018

Uh oh!

Wasabi375 commented Sep 6, 2018

Uh oh!

Sylvyrfysh commented Sep 7, 2018 via email

Uh oh!

Wasabi375 commented Sep 7, 2018

Uh oh!

Sylvyrfysh commented Sep 7, 2018

Uh oh!

Wasabi375 commented Sep 7, 2018

Uh oh!

elect86 commented Sep 7, 2018

Uh oh!

elect86 commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sylvyrfysh commented Sep 7, 2018

Uh oh!

Sylvyrfysh commented Sep 7, 2018

Uh oh!

elect86 commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sylvyrfysh commented Sep 8, 2018

Uh oh!

elect86 commented Sep 8, 2018

Wasabi375 Sep 6, 2018 •

edited

Loading

elect86 commented Sep 6, 2018 •

edited

Loading

elect86 commented Sep 7, 2018 •

edited

Loading

elect86 commented Sep 7, 2018 •

edited

Loading