Replace ASSERT with CORRADE_ASSERT in PTexMeshData.cpp #225

bigbike · 2019-09-19T00:08:40Z

If you look into the implementation of ASSERT, you will find that it actually cannot output any error message when the condition is failed.

Apply the CORRADE_ASSERT in the Corrade library instead.

In this PR:
-) replaced all the ASSERT with CORRADE_ASSERT;
-) added "Error: " to some of the error messages;

Motivation and Context

How Has This Been Tested

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

If you look into the implementation of ASSERT, you will find that it actually cannot output error message when the condition is failed. Apply the CORRADE_ASSERT shipped with Corrade library instead. In this PR: -) replaced all the ASSERT with CORRADE_ASSERT; -) added "Error: " to some of the error messages;

bigbike · 2019-09-19T00:11:50Z

This PR is ready to be reviewed. Thanks.

msavva

Thanks for catching this @bigbike . Let's create an issue to replace use of ASSERT with CORRADE_ASSERT globally in a separate PR.

mosra · 2019-09-19T16:20:21Z

src/esp/assets/PTexMeshData.cpp

@@ -512,7 +538,7 @@ void PTexMeshData::parsePLY(const std::string& filename,
      colorOffsetBytes = offsetSoFarBytes;
      offsetSoFarBytes += colorBytes;
    } else {
-      ASSERT(false);
+      CORRADE_ASSERT(false, "Error: unknown.", );


CORRADE_ASSERT_UNREACHABLE() would be more descriptive in this case I think.

Good to know. will change.

mosra · 2019-09-19T16:22:36Z

src/esp/assets/PTexMeshData.cpp

+                   "Error: number of vertices is not greater than 0", );
+    CORRADE_ASSERT(
+        positionDimensions > 0,
+        "Error: the dimensions of the position is not greater than 0", );


Is the Error: prefix really needed? :) In every case it's the last thing you see before it aborts.

In Magnum itself I'm instead prefixing every assert with class/function name to make it easier to figure out where it blew up. In this case it would be e.g. PTexMeshData::parsePly(): the dimensions....

(Also, I'm planning to add configurable message prefixes / coloring in the near future.)

Good suggestion. I used to do what you recommended as well, and then I stopped to do so since the ASSERT, which used LOG, would output the filename and line number.

@mosra: Another question to you about the CORRADE_ASSERT, it seems it would generate a run-time error instead of exiting the program (like e.g., exit(-1) does)?

A bit more background on this: there's CORRADE_INTERNAL_ASSERT() which doesn't have a message but OTOH prints file/line info. That one is designed for internal assertions which shouldn't ever fire as a result of bad API usage by the library user -- and since the internal error messages would be in most cases just "internal state that's too complex to explain in a single sentence is totally messed up, sorry for your bad experience" I decided to not have them there.

OTOH I thought showing file/line info in CORRADE_ASSERT() would be mostly useless for the user as they rather need to know what function name caused everything to blow up (as opposed to knowing it's line 2765 of AbstractTexture.cpp, for example). So for those there's just a human-readable formatted message.

Basically when deciding which of them to use, I think whether the assertion can happen rather from bad usage on the API user side (in which case it's more important to show why is it wrong), or it's just an internal sanity check that shouldn't hit when just passing bad parameters to a function (in which case file/line info is better). ... Hope those design decisions still make sense now :)

it seems it would generate a run-time error instead of exiting the program (like e.g., exit(-1) does)?

Yes, it calls std::abort() instead of std::exit() -- reason is that the former generates a proper backtrace (or enters a debugger on some systems), which is what you usually want when an assert hits. The exit() would just exit without giving you any chance to know from where it was fired.

OK, I see your point on the design decisions. I totally understand you.

To decide which one to use, we need to think about the purpose of the error message (to expedite debugging or FYI only), and who is the audience (the developer or the user of the system).

Though it is pretty vague in some cases, it is not hard to determine.

I will go over all the places I changed, and update accordingly.

Oh, you used std::abort(). That is why.

Yes, the returned backtrace is rather useful sometimes for the developer.

But again, if the audience is a user of our simulator, in my humble opinion, it is more comfortable for her to see the program is terminated normally (exit()) instead of abnormally (abort()).

But it didn't terminate normally -- It terminated due to a failed assertion!

Yes, indeed, that is actually what happened. I agree, but that is just from the developer's perspective.

But from a user's view, maybe it is more comfortable (or friendly) if the system can "pretend" to quit "normally",meaning it exits without a run-time error (otherwise, there would be an annoying pop-up dialog-box on mac e.g.,). Just my opinion.

I definitely don't think people want things to crash with no indication that anything went wrong. It seems very hard for them to figure out that something is broken if we don't inform them in a fairly loud way.

If you want to exit() with a message, use Fatal{} << blah;. It exits the "normal" way, it's what Utility::Arguments do when you pass a wrong thing on the command line. Asserts are loud by definition and yes, are meant for developers. If a non-developer hits an assert, it's the app developers fault this wasn't checked and handled gracefully.

Hmmm. Maybe something like FATAL_IF(<blah>, <msg>) would be useful? Some of the stuff here is checking if a file exists, so a user could in theory hit that. The assert style synatx is nice than if (<blah>) Fatal{} << <msg>; IMO

As I understand it, asserts are macros mainly so there's a possibility to compile them out in optimized builds. Apart from that I'm trying to avoid macros as much as possible, so the non-macroy if() is no problem for me.

-) replaced "Error" with class::functionName at the beginning of every error message; -) used CORRADE_ASSERT_UNREACHABLE;

bigbike · 2019-09-19T23:28:40Z

Code is updated. Let me know if you have more concerns. Thanks.

src/esp/assets/PTexMeshData.cpp

bigbike · 2019-09-23T22:48:18Z

Let me know if you have more concerns before I merge it in.

mosra · 2019-09-24T10:15:50Z

src/esp/assets/PTexMeshData.cpp

-  ASSERT(submeshID >= 0 && submeshID < renderingBuffers_.size());
+  CORRADE_ASSERT(submeshID >= 0 && submeshID < renderingBuffers_.size(),
+                 "PTexMeshData::uploadBuffersToGPU: the submesh ID "
+                     << submeshID << " is out of range.",


Suggested change

<< submeshID << " is out of range.",

<< submeshID << "is out of range.",

Neither with Debug / Warning / Error / Fatal nor with the assert macros you need to wrap values with spaces, that's done automatically to save you extra typing. Putting the space there would make it doubly-spaced. Docs.

Same in all other cases ;)

bigbike · 2019-09-24T23:13:00Z

Thanks all reviewers.

bigbike · 2019-09-25T23:00:21Z

src/esp/assets/PTexMeshData.cpp


  // Parse parameters
  const auto& paramsFile = atlasFolder + "/parameters.json";
-  ASSERT(io::exists(paramsFile));
+  if (io::exists(paramsFile)) {


This is a bug.

Bump version to v0.1.3

…rch#225) * Replce ASSERT with CORRADE_ASSERT in PTexMeshData.cpp If you look into the implementation of ASSERT, you will find that it actually cannot output error message when the condition is failed. Apply the CORRADE_ASSERT shipped with Corrade library instead. In this PR: -) replaced all the ASSERT with CORRADE_ASSERT; -) added "Error: " to some of the error messages; * minor * address reviewer's concerns. -) replaced "Error" with class::functionName at the beginning of every error message; -) used CORRADE_ASSERT_UNREACHABLE; * minor * use Fatal{} instead of ASSERT at certain places * remove space * minor * minor * minor

bigbike requested review from mosra, msavva, erikwijmans and msbaines September 19, 2019 00:08

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 19, 2019

minor

eef7bcc

msavva approved these changes Sep 19, 2019

View reviewed changes

mosra reviewed Sep 19, 2019

View reviewed changes

bigbike added 2 commits September 19, 2019 16:10

address reviewer's concerns.

18abdf4

-) replaced "Error" with class::functionName at the beginning of every error message; -) used CORRADE_ASSERT_UNREACHABLE;

minor

e9a3c17

bigbike closed this Sep 23, 2019

bigbike force-pushed the replica_assert branch from e9a3c17 to 9bf5cc7 Compare September 23, 2019 21:21

Merge branch 'master' into replica_assert

27e16f0

bigbike reopened this Sep 23, 2019

use Fatal{} instead of ASSERT at certain places

7716200

bigbike commented Sep 23, 2019

View reviewed changes

src/esp/assets/PTexMeshData.cpp Outdated Show resolved Hide resolved

mosra reviewed Sep 24, 2019

View reviewed changes

bigbike added 4 commits September 24, 2019 12:05

remove space

b6324e1

minor

f6a09e1

minor

c7d707d

minor

d5cd589

bigbike merged commit f433ea7 into master Sep 24, 2019

bigbike deleted the replica_assert branch September 24, 2019 23:13

bigbike commented Sep 25, 2019

View reviewed changes

bigbike added a commit that referenced this pull request Sep 25, 2019

fix a bug introduced by PR #225

fc8c109

bigbike added a commit that referenced this pull request Sep 25, 2019

fix a bug introduced by PR #225 (#249)

d5a0bc1

bigbike added this to the Replica PTex Rendering Support milestone Oct 14, 2019

eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020

Bump version to v0.1.3 (facebookresearch#225)

7f2c404

Bump version to v0.1.3

Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020

fix a bug introduced by PR facebookresearch#225 (facebookresearch#249)

7d98ee8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ASSERT with CORRADE_ASSERT in PTexMeshData.cpp #225

Replace ASSERT with CORRADE_ASSERT in PTexMeshData.cpp #225

bigbike commented Sep 19, 2019

bigbike commented Sep 19, 2019

msavva left a comment

mosra Sep 19, 2019

bigbike Sep 19, 2019

mosra Sep 19, 2019

bigbike Sep 19, 2019

mosra Sep 19, 2019

bigbike Sep 19, 2019

bigbike Sep 19, 2019 •

edited

Loading

bigbike Sep 19, 2019 •

edited

Loading

erikwijmans Sep 20, 2019 •

edited

Loading

mosra Sep 20, 2019

erikwijmans Sep 20, 2019

mosra Sep 20, 2019

bigbike commented Sep 19, 2019

bigbike commented Sep 23, 2019

mosra Sep 24, 2019

bigbike commented Sep 24, 2019

bigbike Sep 25, 2019

	<< submeshID << " is out of range.",
	<< submeshID << "is out of range.",

Replace ASSERT with CORRADE_ASSERT in PTexMeshData.cpp #225

Replace ASSERT with CORRADE_ASSERT in PTexMeshData.cpp #225

Conversation

bigbike commented Sep 19, 2019

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

bigbike commented Sep 19, 2019

msavva left a 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

Choose a reason for hiding this comment

bigbike Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

bigbike Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

erikwijmans Sep 20, 2019 • 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

bigbike commented Sep 19, 2019

bigbike commented Sep 23, 2019

Choose a reason for hiding this comment

bigbike commented Sep 24, 2019

Choose a reason for hiding this comment

bigbike Sep 19, 2019 •

edited

Loading

bigbike Sep 19, 2019 •

edited

Loading

erikwijmans Sep 20, 2019 •

edited

Loading