Skip to content
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.

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

Misc #640

Merged
merged 4 commits into from
Dec 9, 2023
Merged

Misc #640

merged 4 commits into from
Dec 9, 2023

Conversation

pca006132
Copy link
Collaborator

  1. Makes tracy support better.
  2. Remove some warnings from clang and clang-tidy.

image

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks! Did this find anything interesting so far?

@@ -13,26 +13,17 @@
// limitations under the License.

#include "polygon.h"
#if MANIFOLD_PAR == 'T'
#include "tbb/tbb.h"
Copy link
Owner

Choose a reason for hiding this comment

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

💯


// Helper function used to swap X with Y and Y with X if c == true
void CondSwap(bool c, float& X, float& Y) {
inline void CondSwap(bool c, float& X, float& Y) {
Copy link
Owner

Choose a reason for hiding this comment

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

Ah yes, I forgot about this.

@@ -289,7 +289,7 @@ struct SVDSet {
*
* @param A The matrix to decompose.
*/
SVDSet SVD(glm::mat3 A) {
inline SVDSet SVD(glm::mat3 A) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should we move this into a .cpp? I'm not completely clear on the tradeoffs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not much pros and cons, more like ideological issue. If this is complicated and used in multiple places, putting it in a .cpp will reduce compilation time a bit, but it is not the case here.

#define FrameMarkStart(x)
#define FrameMarkEnd(x)
#define ZoneScoped
#define ZoneScopedN(name)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe a good place to document how we should use ZoneScoped? I have a vague idea from your usage, but I'm not entirely sure what it does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, it is just something to mark the current zone (scope) for the profiler to see.

@@ -144,6 +144,7 @@ Contributions are welcome! A lower barrier contribution is to simply make a PR t

There is now basic support for the [Tracy profiler](https://github.com/wolfpld/tracy) for our tests.
To enable tracing, compile with `-DTRACY_ENABLE=on` cmake option, and run the test with Tracy server running.
To enable memory profiling in addition to tracing, compile with `-DTRACY_MEMORY_USAGE=ON` in addition to `-DTRACY_ENABLE=ON`.
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@pca006132
Copy link
Collaborator Author

No, clang-tidy did not find anything interesting.

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (45826d6) 91.62% compared to head (1c38c7c) 91.43%.

Files Patch % Lines
src/utilities/include/svd.h 63.63% 4 Missing ⚠️
src/manifold/src/manifold.cpp 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
- Coverage   91.62%   91.43%   -0.20%     
==========================================
  Files          36       36              
  Lines        4682     4656      -26     
==========================================
- Hits         4290     4257      -33     
- Misses        392      399       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pca006132 pca006132 merged commit c9322ca into elalish:master Dec 9, 2023
@pca006132 pca006132 deleted the misc branch December 9, 2023 07:40
@elalish elalish mentioned this pull request Jan 7, 2024
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* update tracy

* fix warnings

* remove dead code and warnings

* add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants