Read write obj #35

acrlakshman · 2017-07-06T03:09:26Z

Please pick a relevant checklist(remove others) and make sure to finish & check all items for the PR.

Feature Completeness:

Algorithm Implementation
Unit tests

acrlakshman · 2017-07-06T03:10:56Z

@9prady9 From next time I will keep updating the same branch instead of deleting it. Sorry, I realized it late.

acrlakshman · 2017-07-06T13:32:30Z

include/meshio/utilities.hpp

+static inline bool isEqual(const T pA, const T pB, const T pTolerance = 1e-6)
+{
+    if (std::abs(pA - pB) <= pTolerance)
+        return true;


@9prady9 this is perfectly valid as per https://google.github.io/styleguide/cppguide.html#Conditionals. Do we need to change this by following the convention that codacy-bot tells? or is there a way to suppress this by making this style an exception?

acrlakshman · 2017-07-06T13:35:07Z

include/meshio/meshio_defines.hpp

+                    return false;
+        if (mNormals.size())
+            for (unsigned i = 0; i < mNormals.size(); ++i)
+                if (!(mNormals[i] == pGeometryDataObj.mNormals[i]))


@9prady9 This way of writing is also correct as per google coding style. As an example it is present in the code snippet just above https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

What do you suggest?

acrlakshman · 2017-07-08T21:48:49Z

include/meshio/vectors.hpp

+
+    std::size_t size() const { return 2; }
+    viterator end() { return (Vec4<T>::end()-2); }
+    const_viterator end() const { return (Vec4<T>::end()-2); }


@9prady9 I couldn't get this line to be covered in test cases. Please take a look at TODO comment in test/utilities.cpp

That's fine. Aren't we use Vec2 anywhere ? Lets just comment this out if we are not using it currently.

@9prady9 Currently we are not using Vec2, except in unit tests. I will comment out this line. But I could not understand why this is not covered though I explicitly tried to use this in unit tests test/utilities.cpp

Not sure, but if we are not using this yet. Lets just comment it out and re-enable it when it can be used. Don't delete it though, just comment it out.

acrlakshman · 2017-07-08T21:55:53Z

include/meshio/vectors.hpp

+    Vec4(T pX, T pY, T pZ, T pW) : elems({pX, pY, pZ, pW}) {}
+    Vec4() : Vec4(0, 0, 0, 1) { } /* TODO: In graphics applications we
+                                     generally use fourth point as 1.
+                                     If this is not OK we can change it */


@9prady9 Please take a look at this comment too. If this needs to be changed back to (0,0,0,0), then I need to change test case. However, I wanted to know which one we should use. Generally fourth parameter we use as 1 in graphics for positions. Please correct me if I am wrong.

I think that is fine especially since we do provide constructor where the user can set the vector components explicitly to which ever values he wants to.

acrlakshman · 2017-07-08T21:57:20Z

@9prady9 Code is good for review

9prady9 · 2017-07-10T11:38:04Z

include/meshio/obj.hpp

+};
+
+template<class T>
+class OBJData {


Can't we have OBJAttribute merged into OBJData ? Why two classes ?

@9prady9

Here, we need two vectors (i) that stores geometric attributes, (ii) material information. If I try to use one derived class from GeometryData<T>, to store entire obj information, I am unable to differentiate geometry and material information that we get from tinyobjloader. So, I introduced one derived class of GeometryData<T> and used vector of this derived class in OBJData. So, instead of using something similar to vector<STLData>, I used vector<mOBJAttribute>. It is getting difficult for me to use one class in this case.

I think it is better to have an class defined and accessible only from OBJData instead of a class that is accessible from everywhere. It would go something like below

class OBJData { typedef struct OBJAttributes { // ... } Attributes; };

That way you can isolate tinyobjloader contents into a separate container class and also avoid creating an user-facing class - since meshio is header-only we have avoid anything and everything that can cause unintended usage by end users. In this case, they may try to use OBJAttributes. However, if the definition of this container is limited to the scope of OBJData class, then they can't even access it in the first place.

Yes, thank you. I will do it again.

9prady9 · 2017-07-10T11:39:24Z

include/meshio/obj.hpp

+        }
+    }
+
+    bool isEqualOBJMaterials(const OBJData<T>& pOBJData) {


May be a name like areMaterialsSame can give more readable code ? OBJ doesn't need to be part of this name I think because this is method of OBJ related class object.

9prady9 · 2017-07-10T11:40:13Z

include/meshio/obj.hpp

+        mOBJMaterials.clear();
+    }
+
+    bool isEqualOBJAttributes(const OBJData<T>& pOBJData) {


May be a name like areAttributesSame can give more readable code ? OBJ doesn't need to be part of this name I think because this is method of OBJ related class object.

9prady9 · 2017-07-10T11:41:25Z

include/meshio/obj.inl

+ * TODO: Thorough testing needed for this function
+ */
+template<typename T>
+bool WriteMtl(const char* pMtlFileName,


Lets go with writeMaterial. I don't remember exactly, didn't we agree for camel case for function naming?

@9prady9 My mistake I will update this.

9prady9 · 2017-07-10T11:44:43Z

include/meshio/utilities.hpp

+/**
+ * Extracts filename from path.
+ */
+static inline std::string getFileBaseName(const std::string& pFileName)


Shouldn't this return the last word found in the path? Even that is a valid file name. All files doesn't need to have an extension necessarily. If you intend to have this function not handle such cases, mention that in function documentation to be clear for whoever might use this in future. May be in a section such as limitations in the function docs.

@9prady9 I need a function that returns base name for file with file_name.obj, so that I can use it to write file_name.mtl. So, this is aimed for writing obj files and its derivative files. This is why I had it like this, any suggestion in this regard? At the end I do need a function that returns basename and it does expect an extension.

I will mention in the documentation too and I will include this when I update this patch.

9prady9 · 2017-07-10T11:45:41Z

include/meshio/vectors.hpp

+    Vec4(T pX, T pY, T pZ, T pW) : elems({pX, pY, pZ, pW}) {}
+    Vec4() : Vec4(0, 0, 0, 1) { } /* TODO: In graphics applications we
+                                     generally use fourth point as 1.
+                                     If this is not OK we can change it */


I think that is fine especially since we do provide constructor where the user can set the vector components explicitly to which ever values he wants to.

9prady9 · 2017-07-10T11:47:40Z

include/meshio/vectors.hpp

+
+    std::size_t size() const { return 2; }
+    viterator end() { return (Vec4<T>::end()-2); }
+    const_viterator end() const { return (Vec4<T>::end()-2); }


That's fine. Aren't we use Vec2 anywhere ? Lets just comment this out if we are not using it currently.

9prady9 · 2017-07-10T11:51:47Z

test/utilities.cpp

+using namespace meshio;
+
+TEST(VECTORS, MISCELLANEOUS)
+{


May be split tests into TEST(VECTORS, 2D) , TEST(VECTORS, 3D) and 4D if possible. Better readable and easy to debug and when they fail tests.

9prady9 · 2017-07-10T11:52:52Z

@acrlakshman ~~Is tiny OBJ loeader file you placed under third_party how is this being added ? did you copy it to that location and added it to the repo directly ?~~

I checked it myself, may be there is a better way to include this. I am not sure how this works for header-only libraries.

9prady9 · 2017-09-26T03:32:16Z

@acrlakshman Are you done with the changes ?

acrlakshman · 2017-09-26T04:06:41Z

@9prady9 Not yet

* referencing #25, #26 * Rewrote some basic datastructures [e.g. GeometryData]. * Read function for OBJ format completed with the help of tinyobjloader. * Write function for OBJ format completed with the help of tinyobjloader. * Minor cleanups. * Rewrote some components to adhere to Google C++ style guide. * Submitting this patch to receive any comments while WIP. Signed-off-by: Lakshman Anumolu <[email protected]>

Also fixed function naming style in vectors.hpp

* Made end() a virtual function in Vec classes. Signed-off-by: Lakshman Anumolu <[email protected]>

* removed obj subfolder * obj tests working * fixed iterator bug in Vec3 class * updated copyright year Signed-off-by: Lakshman Anumolu <[email protected]>

Signed-off-by: Lakshman Anumolu <[email protected]>

acrlakshman · 2019-04-13T16:49:44Z

@9prady9 I am rewritting read and write of obj separately and submit as different PRs. tiny_obj_loader has changed over the past year, I will use that.

acrlakshman requested a review from 9prady9 July 6, 2017 03:09

acrlakshman self-assigned this Jul 6, 2017

acrlakshman commented Jul 6, 2017

View reviewed changes

Repository owner deleted a comment Jul 7, 2017

Repository owner deleted a comment Jul 8, 2017

acrlakshman commented Jul 8, 2017

View reviewed changes

Repository owner deleted a comment Jul 8, 2017

acrlakshman commented Jul 8, 2017

View reviewed changes

9prady9 requested changes Jul 10, 2017

View reviewed changes

Repository owner deleted a comment Jul 11, 2017

acrlakshman and others added 6 commits March 17, 2019 20:53

Changes to vector classes to be more terse

593f90e

Also fixed function naming style in vectors.hpp

Changes

93160de

* Made end() a virtual function in Vec classes. Signed-off-by: Lakshman Anumolu <[email protected]>

OBJ related changes and unit tests

d1ef035

* removed obj subfolder * obj tests working * fixed iterator bug in Vec3 class * updated copyright year Signed-off-by: Lakshman Anumolu <[email protected]>

updated tiny_obj_loader.h

55db685

Signed-off-by: Lakshman Anumolu <[email protected]>

minor update

91d1912

Signed-off-by: Lakshman Anumolu <[email protected]>

acrlakshman closed this Apr 13, 2019

9prady9 added the invalid label Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read write obj #35

Read write obj #35

acrlakshman commented Jul 6, 2017 •

edited

Loading

acrlakshman commented Jul 6, 2017

acrlakshman Jul 6, 2017

acrlakshman Jul 6, 2017

acrlakshman Jul 8, 2017

9prady9 Jul 10, 2017

acrlakshman Jul 10, 2017 •

edited

Loading

9prady9 Jul 10, 2017

acrlakshman Jul 8, 2017

9prady9 Jul 10, 2017

acrlakshman commented Jul 8, 2017

9prady9 Jul 10, 2017

acrlakshman Jul 11, 2017 •

edited

Loading

9prady9 Jul 11, 2017 •

edited

Loading

acrlakshman Jul 11, 2017

9prady9 Jul 10, 2017

9prady9 Jul 10, 2017

9prady9 Jul 10, 2017

acrlakshman Jul 10, 2017

9prady9 Jul 10, 2017

acrlakshman Jul 10, 2017 •

edited

Loading

9prady9 Jul 10, 2017

9prady9 Jul 10, 2017

9prady9 Jul 10, 2017

9prady9 commented Jul 10, 2017 •

edited

Loading

9prady9 commented Sep 26, 2017

acrlakshman commented Sep 26, 2017

acrlakshman commented Apr 13, 2019

Read write obj #35

Read write obj #35

Conversation

acrlakshman commented Jul 6, 2017 • edited Loading

acrlakshman commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrlakshman Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrlakshman commented Jul 8, 2017

Choose a reason for hiding this comment

acrlakshman Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

9prady9 Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acrlakshman Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9prady9 commented Jul 10, 2017 • edited Loading

9prady9 commented Sep 26, 2017

acrlakshman commented Sep 26, 2017

acrlakshman commented Apr 13, 2019

acrlakshman commented Jul 6, 2017 •

edited

Loading

acrlakshman Jul 10, 2017 •

edited

Loading

acrlakshman Jul 11, 2017 •

edited

Loading

9prady9 Jul 11, 2017 •

edited

Loading

acrlakshman Jul 10, 2017 •

edited

Loading

9prady9 commented Jul 10, 2017 •

edited

Loading