View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028511CommunityOCCT:Modeling Datapublic2017-03-06 16:222019-08-12 12:56
Reporterjensgw Assigned Tomsv 
PrioritynormalSeverityjust a question 
Status feedbackResolutionno change required 
Product Version7.1.0 
Target VersionUnscheduled 
Summary0028511: GProp_GProps missleading method name
DescriptionHi there,

GProp_GProps::Mass() returns either:
a) the mass, or
b) lenghts of edges, or
c) area of faces, or
d) volume of solid.
This is confusing and not at all what one would expect from a method with this name.

If there is no density set i would expect an exception for mass computation.
Maybe a mathod Mass(Density) to set the mass temporarily for the computation would be nice.

Also another methods which return the required value would be awesome: Volume(), Surface(), Length().

And maybe a new method Dimension() which works like Mass() right now leads to less confusion.

Cheers Jens
TagsNo tags attached.
Test case number

Activities

msv

2017-03-07 17:42

developer   ~0064184

I see no confusion here. All is explained in the doxygen comments to the method:

  //! Returns the mass of the current system.
  //! If no density is attached to the components of the
  //! current system the returned value corresponds to :
  //! - the total length of the edges of the current
  //! system if this framework retains only linear
  //! properties, as is the case for example, when
  //! using only the LinearProperties function to
  //! combine properties of lines from shapes, or
  //! - the total area of the faces of the current system if
  //! this framework retains only surface properties,
  //! as is the case for example, when using only the
  //! SurfaceProperties function to combine
  //! properties of surfaces from shapes, or
  //! - the total volume of the solids of the current
  //! system if this framework retains only volume
  //! properties, as is the case for example, when
  //! using only the VolumeProperties function to
  //! combine properties of volumes from solids.
  //! Warning
  //! A length, an area, or a volume is computed in the
  //! current data unit system. The mass of a single
  //! object is obtained by multiplying its length, its area
  //! or its volume by the given density. You must be
  //! consistent with respect to the units used.

If you did not use density in the method Add then 1 is assumed.

No changes are required.

msv

2017-03-07 17:42

developer   ~0064185

Dear bugmaster, please close this issue.

jensgw

2017-03-07 18:12

reporter   ~0064186

Yes the behaviour is explained in the doxygen. However the concepts of Mass, Volume, Area and Lenght are distinct dimensions (see SI Unit system, https://en.wikipedia.org/wiki/SI_base_unit) - you would not expect to get a volume if you ask for a mass, and the other way around.
Logically the behaviour makes no sense. Consider this real world examples.
Ask for a mass:
"Hey buddy, how heavy is your new car?" -> "Well, thank you for asking, considering a density of 1 kg/m^3, it has a volume of 5000 mm^3."
Or the other way around, ask for a volume:
"How much volume does this car take up, considered its density is 1.2 kg/m^3 ?" -> "It weighs about 1000 kg."

Also, good naming greatly improves readability and understanding of the source code.
Some Pseudocode:
1 GProp_GProps system = new GProp_GProps();
2 BRepGProp.VolumeProperties(someShape, system);
3 double volume = system.Mass();

This very much looks like a typing error at first clance. Then you try reafctoring it to:
3 double volume = system.Volume();
And now you discover that ther is no Volume() method.
Now you look into the doc and wonder why you have to ask for a mass when you want to have a volume.
What about when density is set. Then you have to unset the density to get the volume. Come on...

This got rather wordy, occt allready provides great possiblities, this request strives to improve it even further.

abv

2017-03-09 12:34

manager   ~0064197

Apparently the class GProp_GProps was designed with intent to hold actual mass properties of physical objects, which may be represented by models of different dimensions in CAD (volumes, surfaces, linear, or even points). Thus it allows you to add multiple contributions and compute their common mass, regardless of whether they have the same dimension or not.

The real problem, in my view, is that we do not use it as intended; instead we use it to calculate particular properties of CAD objects (volume, area, or length).

I fully agree that term "Mass" is quite confusing in this context. Possible alternatives could be "Value", "Size", "Extent", "Measure". If we agree on better name (one of these or another), we can add it to replace Mass(), and declare Mass() as deprecated.

Perhaps the right way to go would be to add additional parameter indicating type of the property: Mass (as it is now), Volume, Area, Length, Points. Functions that calculate geometric properties should then set the type accordingly; mass can still be calculated by combining geometric properties with density specified for each. Method Add() shall have separate implementation for the case when density is specified (it should check that current object is Mass and argument is of geometric type) and not (it should check that current object and argument have the same type). Dedicated methods to get area, volume, length, and points count can be added then -- these should raise error if type of the object does not correspond to requested quantity.

jensgw

2017-03-09 13:09

reporter   ~0064201

+1 :-)

jensgw

2018-05-02 18:30

reporter   ~0075756

Giving this a bump, since it is still a mess.

abv

2018-05-03 09:01

manager   ~0075757

Currently this issue is not a priority, thus it can take long time to get it attended by the core team. Please consider contributing.

Issue History

Date Modified Username Field Change
2017-03-06 16:22 jensgw New Issue
2017-03-06 16:22 jensgw Assigned To => msv
2017-03-07 17:42 msv Note Added: 0064184
2017-03-07 17:42 msv Note Added: 0064185
2017-03-07 17:42 msv Assigned To msv => bugmaster
2017-03-07 17:42 msv Status new => feedback
2017-03-07 17:42 msv Resolution open => no change required
2017-03-07 17:43 msv Severity minor => just a question
2017-03-07 18:12 jensgw Note Added: 0064186
2017-03-09 12:34 abv Note Added: 0064197
2017-03-09 12:35 abv Assigned To bugmaster => msv
2017-03-09 13:09 jensgw Note Added: 0064201
2018-05-02 18:30 jensgw Note Added: 0075756
2018-05-03 09:01 abv Note Added: 0075757
2018-05-03 09:01 abv Target Version => 7.4.0
2019-08-12 12:56 msv Target Version 7.4.0 => Unscheduled