I634 progress bar in export by ABSitf · Pull Request #300 · MeshInspector/MeshLib

ABSitf · 2022-07-22T17:05:46Z

No description provided.

source/MRViewer/MRViewerIO.h

Grantim · 2022-07-25T09:05:52Z

source/MRViewer/MRViewerIO.h

+extern "C" {
+
+MRVIEWER_API EMSCRIPTEN_KEEPALIVE int load_files( int count, const char** filenames );
+
+MRVIEWER_API EMSCRIPTEN_KEEPALIVE int save_file( const char* filename );
+
+MRVIEWER_API EMSCRIPTEN_KEEPALIVE int save_scene( const char* filename );
+
+}
+#endif


this is not needed in header

Grantim · 2022-07-25T09:11:10Z

source/MRMesh/MRProgressCallback.h

+const ProgressCallback emptyProgressCallback = [] ( float )
+{
+    return true;
+};


please remove it

Grantim · 2022-07-25T09:11:20Z

source/MRMesh/MRVoxelsSave.h

+MRMESH_API tl::expected<void, std::string> saveRAW( const std::filesystem::path& path, const ObjectVoxels& voxelsObject,
+                                                    ProgressCallback callback = emptyProgressCallback );


callback = {}

Grantim · 2022-07-25T09:12:54Z

source/MRMesh/MRVoxelsSave.cpp

+    if ( !callback( 0.03f ) )
+        return tl::make_unexpected( std::string( "Saving canceled" ) );


please add callbacks also for actual saving

Grantim · 2022-07-27T07:58:52Z

source/MRViewer/MRViewerIO.cpp

+    if ( !callback( 0.f ) )
+        return "Saving canceled";


if ( callback && !callback( 0.f ) )

source/MRMesh/MRVoxelsSave.cpp

Grantim · 2022-07-27T08:06:40Z

source/MRMesh/MRProgressReadWrite.cpp

+        if ( callback && !callback( float( blockIndex * blockSize ) / dataSize ) )
+            return false;


no need to check callback existance, we checked it in the beggining of this function

source/MRMesh/MRPointsSave.cpp

Grantim · 2022-07-27T08:24:49Z

source/MRMesh/MRPointsSave.cpp

 #include <fstream>
+#include "MRMesh/MRProgressReadWrite.h"


better add new includes before std ones

source/MRMesh/MRMeshSave.cpp

Grantim · 2022-07-27T09:11:21Z

source/MRMesh/MRPointsSave.cpp

+    saveData.callbackFn = [callback, &saveData] ( float progress )
+    {
+        progress = ( saveData.sum + progress * saveData.blockSize ) / saveData.maxSize;
+        float newProgress = 0.f;
+        for ( int i = 0; i < 5; ++i )
+        {
+            if ( progress < 0.2f )
+            {
+                newProgress += progress / 0.2f * 0.7f * ( 1 - newProgress );
+                break;
+            }
+            else
+            {
+                progress = ( progress - 0.2f ) / 0.8f;
+                newProgress += ( 1 - newProgress ) * 0.7f;
+            }
+        }
+        return callback( newProgress );
+    };


please add comments about this

added comments

Grantim · 2022-07-27T09:11:43Z

source/MRMesh/MRMeshSave.cpp

+    struct SaveData
+    {
+        std::function<bool( float )> callbackFn{};
+        std::ostream* stream;
+        size_t sum{ 0 };
+        size_t blockSize{ 0 };
+        size_t maxSize{ 0 };
+    } saveData;
+    saveData.callbackFn = [callback, &saveData] ( float progress )
+    {
+        progress = ( saveData.sum + progress * saveData.blockSize ) / saveData.maxSize;
+        float newProgress = 0.f;
+        for ( int i = 0; i < 5; ++i )
+        {
+            if ( progress < 0.2f )
+            {
+                newProgress += progress / 0.2f * 0.7f * ( 1 - newProgress );
+                break;
+            }
+            else
+            {
+                progress = ( progress - 0.2f ) / 0.8f;
+                newProgress += ( 1 - newProgress ) * 0.7f;
+            }
+        }
+        return callback( newProgress );
+    };


please add comments

added comments

Fedr · 2022-07-27T12:32:18Z

source/MRViewer/MRViewerIO.h

+ * \brief save visual object (mesh, lines, points or voxels) to file
+ * \param callback - callback function to set progress (for progress bar)
+ */
+MRVIEWER_API std::string saveObjectToFile( const std::shared_ptr<VisualObject>& obj, const std::filesystem::path& filename,


What is returned std::string?

added comment

Fedr · 2022-07-27T12:33:07Z

source/MRViewer/MRViewerIO.h

+ * \brief save visual object (mesh, lines, points or voxels) to file
+ * \param callback - callback function to set progress (for progress bar)
+ */
+MRVIEWER_API std::string saveObjectToFile( const std::shared_ptr<VisualObject>& obj, const std::filesystem::path& filename,


Can we change the first parameter to const VisualObject & obj or even to const Object & obj?

Fedr · 2022-07-27T12:35:18Z

source/MRMesh/MRProgressReadWrite.h

+
+/**
+ * \brief write dataSize bytes from data to out stream by blocks blockSize bytes
+ * \details if progress callback not setted write all data by one block


setted -> set

Fedr · 2022-07-27T12:36:19Z

source/MRMesh/MRProgressReadWrite.h

+/**
+ * \brief write dataSize bytes from data to out stream by blocks blockSize bytes
+ * \details if progress callback not setted write all data by one block
+ * \return false if process was canceled (callback setted and return false )


and what is returned if write fails?

this function does not handle errors

Fedr · 2022-07-27T12:38:34Z

source/MRMesh/MRPointsSave.cpp

    for ( auto v : points.validPoints )
+    {
        out << points.points[v] << "\n";
+        if ( callback && !callback( float( pointIndex ) / pointsNum ) )


It is too expensive to run callback for every point! It shall be done on every 1000 point, for example. Here and in other similar places.

Fedr · 2022-07-27T12:40:54Z

source/MRMesh/MRPointsSave.cpp

+            if ( progress < 0.2f )
+            {
+                newProgress += progress / 0.2f * 0.7f * ( 1 - newProgress );
+                break;
+            }
+            else
+            {
+                progress = ( progress - 0.2f ) / 0.8f;
+                newProgress += ( 1 - newProgress ) * 0.7f;


Please introduce named constants (fewer - better) instead of 0.2, 0.7, 0.8

removed magic numbers

Fedr · 2022-07-27T12:41:10Z

source/MRMesh/MRPointsSave.cpp

+        // calculate full progress in partical-linear scale (we don't know compressed size and it less than real size)
+        progress = ( saveData.sum + progress * saveData.blockSize ) / saveData.maxSize;
+        float newProgress = 0.f;
+        for ( int i = 0; i < 5; ++i )


Fedr · 2022-07-27T12:42:11Z

source/MRMesh/MRPointsSave.cpp

            cVert.r = c.r; cVert.g = c.g; cVert.b = c.b;
-            out.write( (const char*) &cVert, 15 );
+            out.write( ( const char* )&cVert, 15 );
+            if ( callback && !callback( float( v ) / numVertices ) )


again too expensive to invoke callback on every triangle

Fedr · 2022-07-27T12:43:01Z

source/MRMesh/MRMeshSave.cpp

+
+    const bool cancel = !MR::writeByBlocks( out, ( const char* )mesh.points.data(), mesh.points.size() * sizeof( Vector3f ), callback );
+    if ( cancel )
+        return tl::make_unexpected( std::string( "Saving canceled" ) );


canceled or write error?

canceled. writeByBlocks return true only if process was canceled

Fedr · 2022-07-27T16:52:32Z

source/MRViewer/MRViewerIO.h

+ * \param callback - callback function to set progress (for progress bar)
+ * \return empty string if no error or error text
+ */
+MRVIEWER_API std::string saveObjectToFile( const Object& obj, const std::filesystem::path& filename,


Let us better return tl::expected<void, std::string> as in MRMeshSave

Fedr · 2022-07-27T16:54:47Z

source/MRMesh/MRPointsSave.cpp

+    {
        out << points.points[v] << "\n";
+        ++pointIndex;
+        if ( !( pointIndex % 1000 ) && callback && !callback( float( pointIndex ) / pointsNum ) )


better

if ( callback && !( pointIndex % 1000 ) && !callback( float( pointIndex ) / pointsNum ) )

to avoid division when no callback, and in other similar places

ABSitf added 6 commits July 22, 2022 18:38

added progress bars in save object to file functions

5e9e8c1

move some code

3e50a3e

fix

79170fd

fix

38d8b60

fix

c9e476a

fix

7a69d36

Grantim requested changes Jul 25, 2022

View reviewed changes

ABSitf added 6 commits July 25, 2022 17:43

fixes

1c84054

reworked progress in save

209c44d

fix

9ecbe24

add some comment, do some rework

72c5d98

fix

b17cdf6

fix

51dc1b0

Grantim requested changes Jul 27, 2022

View reviewed changes

fix

fabea8c

Grantim approved these changes Jul 27, 2022

View reviewed changes

Fedr reviewed Jul 27, 2022

View reviewed changes

fixes

4daa28a

Fedr reviewed Jul 27, 2022

View reviewed changes

ABSitf added 2 commits July 27, 2022 20:50

fixes

ebeabf7

small fix

a17ce04

Fedr approved these changes Jul 27, 2022

View reviewed changes

fix

936a393

ABSitf merged commit f75d637 into master Jul 27, 2022

ABSitf deleted the i634_progress_bar_in_export branch July 27, 2022 19:12

		MRMESH_API tl::expected<void, std::string> saveRAW( const std::filesystem::path& path, const ObjectVoxels& voxelsObject,
		ProgressCallback callback = emptyProgressCallback );

		if ( !callback( 0.03f ) )
		return tl::make_unexpected( std::string( "Saving canceled" ) );

		if ( callback && !callback( float( blockIndex * blockSize ) / dataSize ) )
		return false;

Conversation

ABSitf commented Jul 22, 2022

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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