8000 Fix MANIFOLD_DEBUG guards for Read/Write OBJ api by starseeker · Pull Request #1226 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix MANIFOLD_DEBUG guards for Read/Write OBJ api #1226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 13, 2025
Merged

Fix MANIFOLD_DEBUG guards for Read/Write OBJ api #1226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions include/manifold/manifold.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,8 @@ class Manifold {

/** @name Debugging I/O
* Self-contained mechanism for reading and writing high precision Manifold
* data. Write functions create special-purpose OBJ files, and Read
* functions read them. Be warned these are not (and not intended to be)
* data. Write function creates special-purpose OBJ files, and Read function
* reads them in. Be warned these are not (and not intended to be)
* full-featured OBJ importers/exporters. Their primary use is to extract
* accurate Manifold data for debugging purposes - writing out any info
* needed to accurately reproduce a problem case's state. Consequently, they
Expand All @@ -447,9 +447,6 @@ class Manifold {
* that the ReadOBJ method in a given build/release will read in the output
* of the WriteOBJ method produced by that release.
*
* If Manifold is compiled without MANIFOLD_DEBUG set, ReadOBJ will return
* an invalid Manifold and WriteOBJ will be a no-op returning false;
*
* To work with a file, the caller should prepare the ifstream/ostream
* themselves, as follows:
*
Expand Down Expand Up @@ -480,8 +477,10 @@ class Manifold {
* ofile.close();
* @endcode
*/
#ifdef MANIFOLD_DEBUG
static Manifold ReadOBJ(std::istream& stream);
bool WriteOBJ(std::ostream& stream) const;
#endif
Copy link
Owner
@elalish elalish Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly curious, from a consumer's perspective - is this more convenient? This way if you write code that uses these and then change your CMake flag, it'll suddenly stop compiling. The old way it would just fail to run. Which do we think is better behavior?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I am a PL guy, so I want more static guarantees. But from a user perspective, failing the compilation may not be the best way...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the headers are going to be behind the MANIFOLD_DEBUG guard, don't these methods need to be too? Otherwise user code might not compile anyway due to not having the necessary includes?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, the point is to not include the stream definitions, so we do have to guard them this way. I suppose finding out about problems earlier (compilation) is better than later (runtime).


/** @name Testing Hooks
* These are just for internal testing.
Expand Down
9 changes: 0 additions & 9 deletions src/impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,6 @@ std::ostream& operator<<(std::ostream& stream, const Manifold::Impl& impl) {
* Write function.
*/
Manifold Manifold::ReadOBJ(std::istream& stream) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to guard the implementations the same way as the headers?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe pull master and check - I feel like this won't compile as is with MANIFOLD_DEBUG=OFF, or else I'm missing something about how these compilers work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do - there is an ifdef MANIFOLD_DEBUG guarding the whole block on line 685. I forgot to remove it when I added the method internal versions in the previous commit, so I didn't need to re-add it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it - do all the CI tests have MANIFOLD_DEBUG=ON?

#ifdef MANIFOLD_DEBUG
if (!stream.good()) return Invalid();

MeshGL64 mesh;
Expand Down Expand Up @@ -782,9 +781,6 @@ Manifold Manifold::ReadOBJ(std::istream& stream) {
auto m = std::make_shared<Manifold::Impl>(mesh);
if (epsilon) m->SetEpsilon(*epsilon);
return Manifold(m);
#else
return Invalid();
#endif
}

/**
Expand All @@ -794,15 +790,10 @@ Manifold Manifold::ReadOBJ(std::istream& stream) {
* by WriteOBJ should be read back in with ReadOBJ.
*/
bool Manifold::WriteOBJ(std::ostream& stream) const {
#ifdef MANIFOLD_DEBUG
if (!stream.good()) return false;
stream << *this->GetCsgLeafNode().GetImpl();
return true;
#else
return false;
#endif
}

#endif

} // namespace manifold
Loading
0