[WIP] Rework ObjImporter to support more features by Squareys · Pull Request #205 · mosra/magnum

Squareys · 2017-06-08T11:25:58Z

Hello everybody!

I started reimplementing the ObjImporter pretty much a year ago for a University project, since I needed to import .obj files with materials.

While back then I only needed to support the features that I needed for those specfic .obj files, I decided to try to also implement support for all the previously supported features to hopefully get this into upstream someday.

Added features include:

Materials (usemtl and mtllib keywords)
Groups (g keyword)
Mixed primitive meshes
Zero-copy parsing (almost at least?)

Currently alot of tests are failing. The new implementation seems to be especially bad at failing for bad files still. There is quite alot to do still and may take another couple of days for me to get into a reviewable state.

Cheers, Jonathan.

TODOs

Add file callbacks feature

Squareys · 2017-06-08T11:28:22Z

Oh, I guess I should proabably rebase the branch sometime again :D

done

mosra

Hey, first an apology from my side -- my original code was quite terrible so it's hard to improve it. I hope I didn't put you off with those 36 comments, though -- sorry about that.

mosra · 2017-06-08T14:55:12Z

-        std::unique_ptr<File> _file;
+        Containers::Array<char> _in;
+        std::unique_ptr<struct ImporterState> _state;
+        std::string _fileRoot;


I'd vote for keeping just the single unique_ptr containing all the state, maybe even keeping the original File struct forward-declared above. Easier to clean up on close, less includes in the header.

✔️ done

mosra · 2017-06-08T14:56:13Z

 #include <limits>
 #include <sstream>
+#include <tuple>
+#include <stdlib.h>


<cstdlib> .. why is it needed, by the way?

I think that was because of strtol and strtof

✔️ done

mosra · 2017-06-08T15:22:15Z

+
+#include "MagnumPlugins/TgaImporter/TgaImporter.h"
+
+using namespace Corrade::Containers;


Please no using namespace.

🤔 But it makes alot of things more convenient... Aaaand it's a "private" file? Obviously explicitness is often nicer, but is there any other reason for this?

It's not used on that many places ;)

My reasoning: if a namespace is annoyingly long to type, it indicates a problem with the API naming -- it should be shortened. Using using namespace would somewhat hide that problem, but not solve it, and also makes the code a bit more opaque for people that don't know what belongs to the used namespace. I would say that in this case the namespace is still bearable ;)

mosra · 2017-06-08T17:11:08Z

+    ObjMesh* faces = nullptr;
+
+    ObjMeshData()=delete;
+    ObjMeshData(ObjGroup& g, int matId): group(g), materialId(matId) {}


No need to delete default constructors if there are other constructors defined. Also, would make the other one explicit. Similarly below.

✔️ done

mosra · 2017-06-08T17:12:53Z

+}
+
+// find next non-whitespace char and return suffix at that point.
+// param newlineIsWhitespace if "true", '\n' and '\r' are skipped also


Coding style: please use /* */-style comments, the // ones are treated as temporary to-be-removed comments.

I know! :D It is a temporary comment, so I don't forget to put decent wording for other people there.

✔️ done (Cleanly formulated all the helper documentations and checked for Captial letter at comment starts everywhere)

mosra · 2017-06-08T18:43:05Z

-    CORRADE_COMPARE(importer.mesh3DCount(), 1);
-}
-
-void ObjImporterTest::unnamedMesh() {


Why is this removed? Then there's no test case that tests that mesh3DForName("") returns -1

Well, for the empty file, we now don't load anything at all...
I could put a mesh there, and then check the mesh3DForName(""). But why should it be returning -1 anyway?

Ah, I see. Would be nice to have the check for both unnamed object and mesh then.

-1 is because there can be multiple meshes with empty name -- so you can map their ID to an empty name but not the other way around.

mosra · 2017-06-08T18:45:53Z

    void missingTextureCoordinateIndices();

+    void multiMaterialObject();
+


Is there a test case for the actual material data import?

Good point.

mosra · 2017-06-08T18:47:44Z

    }

-    return MeshData3D{*primitive, std::move(indices), {std::move(positions)}, std::move(normals), std::move(textureCoordinates), {}, nullptr};
+    return std::unique_ptr<AbstractMaterialData>(mat);


If the material would be fully parsed in this function instead of in parse(), you wouldn't need the temporary objMat structure I think.

I hope I'm not talking about impossible things here, though ;)

Sure, I guess it would be enough to only parse the material names in parse() and maybe store their location in ArrayView<const char>s instead

mosra · 2017-06-08T18:48:42Z

+        /* Comment line */
+        if(pos[0] == '#') {
+            pos = ignoreLine(pos);
+            pos = skipWhitespaces(pos);


Again, this is done right in the next loop iteration, no need to do this here as well.

Again, no, the ignore Whitespaces ignores words before the keyword (nextWord() expects first character to be non-whitespace, otherwise will return empty string, indicating there is no word at the beginning), versus the other one skips whitespaces after the keyword.

Maybe Wikipedia has it wrong, but I saw some prefix whitespace in the code samples there: https://en.wikipedia.org/wiki/Wavefront_.obj_file#File_format

I personally don't see why there should be such a restriction of no whitespace at line beginning.

I never said it was a restriction, the parser will parse ······\t····p·1 just fine with my current implementation?

Ah! The point is that I ignore those newlines before using nextWord(). Hence the input to that will always be p·1 in the example above, because the others have already been ignored.

the parser will parse ······\t····p·1 just fine with my current implementation?

My point is that this wouldn't work if this would be the very first line (as far as I can see), so I'm suggesting putting the skipWhitespaces() as the very first thing in the loop, instead of very last ;)

Hmm, alright, yeah, I seem to have removed the first skipWhitespace call somewhere, but what you say makes sense

mosra · 2017-06-08T18:51:37Z

+            /* Create dummy mesh for this object so that it gets loaded as ObjectData */
+            _state->meshlessObjects.push_back(object->name);
+        }
+    };


I'm a bit unsure about these inline lambdas -- i'm personally always trying to do such cleanup code only once in the parsing loop, because otherwise the corner-case testing gets quite out of hand. But I didn't stare at the code long enough yet to see how that could be done :)

This will not be easy, will stare at the code for a while and tell you what I think after.

Squareys · 2017-06-09T13:30:30Z

Thanks for the initial review!

I hope I didn't put you off with those 36 comments, though -- sorry about that.

Never! I am always happy to learn! :D

A couple of things were pretty obvious, though, these things would have gotten resolved in an autonomous code review also 😉 I don't think commenting on code stlye/formatting makes too much sense yet, since I didn't do that final cleanup pass myself yet.

I will have time to apply the requested changes over the weekend :)

mosra · 2017-06-13T09:22:19Z

+/* Not using PhongMaterialData, since we may want to set color and texture to
+ * later decide which flags we set. We do not know whether we have a texture
+ * or color beforehand. */
+struct ObjMaterial {


Oh, forgot to say: could the internal Obj* structs be wrapped in an anonymous namespace as well to prevent symbol leakages? Thank you.

✔️ done

mosra · 2017-06-13T09:30:49Z

+
+
+/* The state of the imported generated by openData() */
+struct ImporterState {


Thank you for putting it all in one -- could it be ObjImporter::State or something so it doesn't appear in the global Trade namespace? There is a leftover struct File forward-declaration in the header also, so you could just reuse that line.

Previously the state unique_ptr was reset in doOpen*(), but those two added members in and fileRoot shouldn't be, because they are set before the state is reset. Hence two options:
Either unique_ptr<File> with a new struct File in the Importer state struct (which can be reset independently from the importer state) or revert back to having the two members outside of this struct.

Took me a while to realize what's going on, sorry for the late reply.

but those two added members in and fileRoot shouldn't be, because they are set before the state is reset

I think they don't need to be. One option:

void ObjImporter::doOpenFile(const std::string& filename) { AbstractImporter::doOpenFile(filename); _state->fileRoot = Utility::Directory::path(filename); } void ObjImporter::doOpenData(Containers::ArrayView<const char> data) { _state.reset(new ImporterState); _state->in = Containers::Array<char>{data.size()}; std::copy(data.begin(), data.end(), _state->in.begin()); parse(); }

The above assumes that parse() doesn't need to access the _fileRoot parameter (which it shouldn't, if you defer the material parsing to later, as I suggested). Another option could be this, where the _state is only populated if it's not already there:

void ObjImporter::doOpenFile(const std::string& filename) { _state.reset(new ImporterState); _state->fileRoot = Utility::Directory::path(filename); AbstractImporter::doOpenFile(filename); } void ObjImporter::doOpenData(Containers::ArrayView<const char> data) { if(!_state) _state.reset(new ImporterState); _state->in = Containers::Array<char>{data.size()}; std::copy(data.begin(), data.end(), _state->in.begin()); parse(); }

The AbstractImporter::open*() function implementation is unconditionally calling close() before calling the doOpen() implementations, so you can be always sure that if _state is already populated, it's yours and not some stale one from a previous file.

Signed-off-by: Squareys <squareys@googlemail.com>

sariug · 2019-04-16T14:34:37Z

Hello,

I am trying to open an obj file. Tried it with 3 different version of it. For same obj file:

I tried opening the original
Opened in MeshLab and saved again with Vertex+color information
Opened in MeshLab and saved again without Vertex+color information

None of the worked nicely with Obj viewer plugin.
Errors I have gotten are, respectively:
-Trade::ObjImporter::mesh3D(): error while converting numeric data
-Trade::ObjImporter::mesh3D(): invalid float array size
-Opening file deneme2.obj; Importing mesh 0 ; Cannot load the mesh, skipping

Do you have any other suggestions to handle these issue as well as is there any expected improvement in this plugin ? or any other suggestions to deal with it ?

The obj file looks like :

Regards,

Squareys force-pushed the obj-rework branch from af3aecd to 13d5f87 Compare June 8, 2017 15:52

mosra reviewed Jun 8, 2017

View reviewed changes

mosra reviewed Jun 13, 2017

View reviewed changes

mosra mentioned this pull request Mar 21, 2018

Data import/export plugin improvements #146

Open

Squareys added 5 commits March 28, 2018 14:26

WIP

ba9a2c3

SCRAP: Add super-rudimentary objimporter rework + multiMaterial test

f79d002

Signed-off-by: Squareys <squareys@googlemail.com>

Remove some debugging code.

e87960b

Signed-off-by: Squareys <squareys@googlemail.com>

WIP

77de8df

Signed-off-by: Squareys <squareys@googlemail.com>

[WIP] Address the "easy" review comments

52a1a86

Signed-off-by: Squareys <squareys@googlemail.com>

Squareys force-pushed the obj-rework branch from badd662 to 52a1a86 Compare March 28, 2018 12:38

ObjImporter: Use AnyImageImporter

4503b01

Signed-off-by: Squareys <squareys@googlemail.com>

mosra mentioned this pull request Sep 28, 2019

[Examples Viewer Tutorial] Can't find a supported format #375

Closed


		#include "MagnumPlugins/TgaImporter/TgaImporter.h"

		using namespace Corrade::Containers;

		void missingTextureCoordinateIndices();

		void multiMaterialObject();



		/* The state of the imported generated by openData() */
		struct ImporterState {

Conversation

Squareys commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Squareys commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mosra left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Squareys Jun 9, 2017 • 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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Squareys Jun 9, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Squareys commented Jun 9, 2017

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!

Squareys commented Jun 8, 2017 •

edited

Loading

Squareys commented Jun 8, 2017 •

edited

Loading

Squareys Jun 9, 2017 •

edited

Loading

Squareys Jun 9, 2017 •

edited

Loading