View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028478CommunityOCCT:Foundation Classespublic2017-02-20 13:232017-09-29 16:29
ReporterBenjaminBihler Assigned Tobugmaster  
PrioritynormalSeveritytrivial 
Status closedResolutionfixed 
PlatformMingw64OSWindows 
Product Version7.1.0 
Target Version7.2.0Fixed in Version7.2.0 
Summary0028478: Scope Names Are Swallowed in Message_ProgressSentry Constructors
DescriptionThe constructors call progress->SetName with the task name and afterwards they call progress->NewScope without passing the name. But NewScope(...) will overwrite the name with an empty string, so at the end the name is not set.

If I am correct, then task names have never been treated correctly in Message_ProgressSentry, except if someone has called progressIndicator->SetName(...) later on. This makes me wonder. Did I get something wrong?
Steps To ReproduceCreate a Message_ProgressIndicator, use a Message_ProgressSentry to initialize the task and then display the task name by calling GetScope(1).GetName() of Message_ProgressIndicator. The task name will be empty because of this issue.
TagsNo tags attached.
Test case numberbugs fclasses bug28478, perf fclasses progress

Activities

git

2017-02-20 13:27

administrator   ~0063913

Branch CR28478 has been created by BenjaminBihler.

SHA-1: 6007f4b782fbb8b66fb0e0094c2c00dc8cdb9dd2


Detailed log of new commits:

Author: Benjamin Bihler
Date: Mon Feb 20 11:25:54 2017 +0100

    0028478: Scope Names Are Swallowed in Message_ProgressSentry Constructors
    
    Constructors pass names during scope creation now instead of setting the
    names before creating a new scope.

BenjaminBihler

2017-06-22 17:00

developer   ~0067613

Reminder sent to: abv

Andrey, would you mind having a look at this issue? I have already created a solution for it.

git

2017-08-18 16:12

administrator   ~0069588

Branch CR28478_1 has been created by abv.

SHA-1: 41345d408cd23a4e81f70ecd8b9efa41ac75343a


Detailed log of new commits:

Author: abv
Date: Fri Aug 18 15:05:34 2017 +0300

    0028478: Scope Names Are Swallowed in Message_ProgressSentry Constructors
    
    Setting name of top-level scope in Message_ProgressIndicator to "Step" by default is avoided, for consistency with default behavior for nested scopes.
    
    Tests are added to control output and performance of progress indicator (bugs fclasses bug28478 and perf fclasses progress, respectively).
    
    Implementation of class Draw_ProgressIndicator is improved to update indicator basing on achieved total progress (1% by default) instead of elapsed time since last update.
    
    Method OSD_Chronometer::Restart() is fixed to actually reset the counter.
    
    DRAW command readstl is improved to show progress indicator if configured (by command XProgress).

abv

2017-08-18 16:19

manager   ~0069591

Hello Benjamin,

I beg your pardon for long delay with my reply -- alas, I was totally occupied by urgent tasks until this week, and progress indicator is too tricky subject to give fast answers on it.

Actually there seem to be no problem in the current implementation. I believe you have been confused by recursive logic of the progress indicator (I admit it is not well documented, will try to improve that).

The point is that the name is assigned not to the progress indicator as a whole, but to current scope. The method SetName() and similar provided by Message_ProgressIndicator class are just short-cuts to the current scope; this can be rather misleading, so we likely will remove them in the future.

On each level of recursion, the procedure that gets the scope allocated by its calling process configures it according to its nature: it sets name, range, step, etc. The name is intended to indicate what is being incremented on the current level (like "step 1 of 23"). Then the procedure increments the progress within its scope, and can (optionally) create sub-scopes for nested procedures.

Class Message_ProgressSentry is intended to provide convenient interface to this.

Consider this sample code:

<code>
  // Outer cycle
  Message_ProgressSentry anOuter (aProgress, "Outer", 0, nbOuter, 1);
  for (int i = 0; i < nbOuter && anOuter.More(); i++, anOuter.Next())
  {
    // Inner cycle
    Message_ProgressSentry anInner (aProgress, "Inner", 0, nbInner, 1);
    for (int j = 0; j < nbInner && anInner.More(); j++, anInner.Next())
    {
      // Cycle body
    }
  }
</code>

In the current implementation it will produce desired output (see implementation of class Draw_ProgressIndicator in the branch CR28478_1 -- I have had to modify it to get usable output):

Progress: 0% Outer: 1 / 3
Progress: 17% Outer: 1 / 3 Inner: 1 / 2
Progress: 33% Outer: 1 / 3 Inner: 2 / 2
Progress: 50% Outer: 2 / 3 Inner: 1 / 2
Progress: 67% Outer: 2 / 3 Inner: 2 / 2
Progress: 83% Outer: 3 / 3 Inner: 1 / 2
Progress: 100% Outer: 3 / 3 Inner: 2 / 2

With your change, the output becomes wrong:

Progress: 0% Step: 1 / 3 Outer: 0 / 100
Progress: 17% Step: 1 / 3 Outer: 1 / 2 Inner: 100 / 100
Progress: 33% Step: 1 / 3 Outer: 2 / 2 Inner: 100 / 100
Progress: 50% Step: 2 / 3 Outer: 1 / 2 Inner: 100 / 100
Progress: 67% Step: 2 / 3 Outer: 2 / 2 Inner: 100 / 100
Progress: 83% Step: 3 / 3 Outer: 1 / 2 Inner: 100 / 100
Progress: 100% Step: 3 / 3 Outer: 2 / 2 Inner: 100 / 100

You can see that names get assigned to sub-scopes and do not match the range.

Thus I consider current implementation as correct and going to keep it (apart of some corrections and new tests added, see branch CR28478_1).
Please let me know your opinion on this.

git

2017-08-18 16:45

administrator   ~0069594

Branch CR28478_1 has been updated forcibly by abv.

SHA-1: 34f43b5fefd04883154321be4da0f653bd57c5e5

BenjaminBihler

2017-08-21 09:54

developer   ~0069694

Okay, I see. I have indeed not completely understood how to deal with the scope hierarchy and I have not been aware of Draw_ProgressIndicator. Otherwise I could have learned from its implementation.

So I am actually happy with the progress indicator code (not with the documentation however). ;)

Another minor remark: the documentation of Message_ProgressIndicator states that after resetting, the scale has the name "Step", but you have commented out that line.

abv

2017-08-21 13:23

manager   ~0069709

Yes sure, thank you for noting this! I am going to update this description, will take this remark into account.

git

2017-08-22 10:42

administrator   ~0069733

Branch CR28478_1 has been updated by abv.

SHA-1: d935af5a77ba44535596b2cb058b996be88395fc


Detailed log of new commits:

Author: abv
Date: Tue Aug 22 10:41:59 2017 +0300

    Description of class Message_ProgressIndicator is updated.

git

2017-08-29 10:37

administrator   ~0070030

Branch CR28478_2 has been created by abv.

SHA-1: de929c73591be20901d01e53ce521c96848f20a4


Detailed log of new commits:

Author: abv
Date: Fri Aug 18 15:05:34 2017 +0300

    0028478: Scope Names Are Swallowed in Message_ProgressSentry Constructors
    
    Tests are added to control output and performance of progress indicator (bugs fclasses bug28478 and perf fclasses progress, respectively).
    
    Implementation of class Draw_ProgressIndicator is improved to update indicator basing on achieved total progress (1% by default) instead of elapsed time since last update.
    
    Method OSD_Chronometer::Restart() is fixed to actually reset the counter.
    
    DRAW command readstl is improved to show progress indicator if configured (by command XProgress).
    
    Description of class Message_ProgressIndicator is updated; code example is added in description of Message_ProgressSentry.

abv

2017-08-29 10:39

manager   ~0070031

Please consider branch CR28478_2 for integration. The patch has been tested in its previous state, different only by class comment; see Jenkins job CR28478-master-abv

bugmaster

2017-08-29 12:04

administrator   ~0070036

Combination -
OCCT branch : CR28478_2 - SHA-1: de929c73591be20901d01e53ce521c96848f20a4
Products branch : master
was compiled on Linux, MacOS and Windows platforms and tested on optimize mode.

http://jenkins-test-10.nnov.opencascade.com/view/CR28478-master-abv/

Number of compiler warnings:

OCCT :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

Products :
Linux: 4 (4 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:

Windows:
OCCT
Total CPU difference: 17330.010689098726 / 17295.75286949859 [+0.20%]
Producst
Total CPU difference: 7688.758886599963 / 7720.302288799964 [-0.41%]

Linux:
OCCT
Total CPU difference: 19451.89000000023 / 19456.560000000376 [-0.02%]
Products
Total CPU difference: 7703.40000000009 / 7725.120000000072 [-0.28%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

New test case is OK

git

2017-09-04 18:20

administrator   ~0070183

Branch CR28478 has been deleted by kgv.

SHA-1: 6007f4b782fbb8b66fb0e0094c2c00dc8cdb9dd2

git

2017-09-04 18:20

administrator   ~0070184

Branch CR28478_1 has been deleted by kgv.

SHA-1: d935af5a77ba44535596b2cb058b996be88395fc

git

2017-09-04 18:20

administrator   ~0070185

Branch CR28478_2 has been deleted by kgv.

SHA-1: de929c73591be20901d01e53ce521c96848f20a4

Related Changesets

occt: master 6b55f8e3

2017-08-18 12:05:34

abv


Committer: bugmaster Details Diff		 0028478: Scope Names Are Swallowed in Message_ProgressSentry Constructors

Tests are added to control output and performance of progress indicator (bugs fclasses bug28478 and perf fclasses progress, respectively).

Implementation of class Draw_ProgressIndicator is improved to update indicator basing on achieved total progress (1% by default) instead of elapsed time since last update.

Method OSD_Chronometer::Restart() is fixed to actually reset the counter.

DRAW command readstl is improved to show progress indicator if configured (by command XProgress).

Description of class Message_ProgressIndicator is updated; code example is added in description of Message_ProgressSentry.		 Affected Issues
0028478
mod - src/Draw/Draw_ProgressIndicator.cxx Diff File
mod - src/Draw/Draw_ProgressIndicator.hxx Diff File
mod - src/Message/Message_ProgressIndicator.hxx Diff File
mod - src/Message/Message_ProgressSentry.hxx Diff File
mod - src/OSD/OSD_Chronometer.cxx Diff File
mod - src/QABugs/QABugs_11.cxx Diff File
mod - src/XSDRAWSTLVRML/XSDRAWSTLVRML.cxx Diff File
add - tests/bugs/fclasses/bug28478 Diff File
add - tests/perf/fclasses/progress Diff File

Issue History

Date Modified Username Field Change
2017-02-20 13:23 BenjaminBihler New Issue
2017-02-20 13:23 BenjaminBihler Assigned To => BenjaminBihler
2017-02-20 13:27 git Note Added: 0063913
2017-02-20 13:27 BenjaminBihler Assigned To BenjaminBihler => mpv
2017-02-20 13:27 BenjaminBihler Status new => resolved
2017-02-20 13:46 kgv Assigned To mpv => abv
2017-02-20 13:46 kgv Category OCCT:Application Framework => OCCT:Foundation Classes
2017-06-22 17:00 BenjaminBihler Note Added: 0067613
2017-08-18 16:12 git Note Added: 0069588
2017-08-18 16:19 abv Note Added: 0069591
2017-08-18 16:45 git Note Added: 0069594
2017-08-21 08:41 abv Assigned To abv => BenjaminBihler
2017-08-21 08:41 abv Status resolved => feedback
2017-08-21 09:54 BenjaminBihler Note Added: 0069694
2017-08-21 10:55 BenjaminBihler Assigned To BenjaminBihler => abv
2017-08-21 13:23 abv Note Added: 0069709
2017-08-22 10:42 git Note Added: 0069733
2017-08-29 10:37 git Note Added: 0070030
2017-08-29 10:39 abv Note Added: 0070031
2017-08-29 10:39 abv Assigned To abv => bugmaster
2017-08-29 10:39 abv Status feedback => reviewed
2017-08-29 12:00 bugmaster Test case number => bugs fclasses bug28478
2017-08-29 12:04 bugmaster Note Added: 0070036
2017-08-29 12:04 bugmaster Status reviewed => tested
2017-08-29 15:30 bugmaster Test case number bugs fclasses bug28478 => bugs fclasses bug28478, perf fclasses progress
2017-08-31 18:37 bugmaster Changeset attached => occt master 6b55f8e3
2017-08-31 18:37 bugmaster Status tested => verified
2017-08-31 18:37 bugmaster Resolution open => fixed
2017-09-04 18:20 git Note Added: 0070183
2017-09-04 18:20 git Note Added: 0070184
2017-09-04 18:20 git Note Added: 0070185
2017-09-29 16:17 aiv Fixed in Version => 7.2.0
2017-09-29 16:29 aiv Status verified => closed