-
-
Notifications
You must be signed in to change notification settings - Fork 678
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
First attempt at taxonomy::implicit_item #3827
base: v0.8.0
Are you sure you want to change the base?
Changes from 1 commit
b51b451
5da9eea
622a21c
dbea6d0
906f824
c3c6402
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,32 +27,11 @@ using namespace ifcopenshell::geometry; | |
|
||
#include <boost/mpl/vector.hpp> | ||
#include <boost/mpl/for_each.hpp> | ||
#include <boost/math/quadrature/trapezoidal.hpp> | ||
|
||
// @todo use std::numbers::pi when upgrading to C++ 20 | ||
#define PI 3.1415926535897932384626433832795 | ||
|
||
|
||
namespace | ||
{ | ||
// trapezoid rule integration | ||
// @todo is there a well established math library we can use instead of | ||
// creating our own integrator? | ||
double integrate(double a, double b, unsigned n, std::function<double(double)> fn) | ||
{ | ||
double area = 0; | ||
double h = (b - a) / n; | ||
for (auto i = 1; i <= n; i++) | ||
{ | ||
auto x1 = a + h * (i - 1); | ||
auto x2 = a + h * i; | ||
auto f1 = fn(x1); | ||
auto f2 = fn(x2); | ||
area += h * (f1 + f2) / 2.0; | ||
} | ||
return area; | ||
} | ||
} | ||
|
||
typedef boost::mpl::vector< | ||
IfcSchema::IfcLine | ||
#ifdef SCHEMA_HAS_IfcClothoid | ||
|
@@ -65,19 +44,25 @@ typedef boost::mpl::vector< | |
, IfcSchema::IfcCircle | ||
> curve_seg_types; | ||
|
||
enum segment_type_t { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this, coupled with the if-else if-else blocks for each IfcCurve type, would it be cleaner to have three different map functions for IfcCurveSegment? map_horizontal(), map_vertical(), map_cant(). These would get called from IfcCompositeCurve, IfcGradiantCurve, and IfcSegmentedReferenceCurve, respectively. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be okay with it as it, but admittedly at this point I'm just hanging on for dear life trying to keep up with the expert-level C++ exchanges between you & @aothms 🤣 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing to think about this, the original approach is better than my idea. My idea would have, for each curve type, three implementations in three different locations. @aothms Original idea has everything for each curve type together. That approach is more clear and maintainable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the same doubts myself. I would have also preferred a more statically typed set of three tuples of types for example. I think the approach now is likely to result in the least lines of code, but it's indeed unclear what is implemented and supported and potentially risky in the sense that a parent curve can be used in an interpretation that's not supported. Combined with returning Eigen::VectorXd there is actually very little typing here to prevent issues. I would propose to keep as is while prototyping, but I think we might have to change it in the future. |
||
ST_HORIZONTAL, ST_VERTICAL, ST_CANT | ||
}; | ||
|
||
class curve_segment_evaluator { | ||
private: | ||
double length_unit_; | ||
double start_; | ||
double length_; | ||
segment_type_t segment_type_; | ||
IfcSchema::IfcCurve* curve_; | ||
|
||
std::optional<std::function<Eigen::Vector3d(double)>> eval_; | ||
std::optional<std::function<Eigen::VectorXd(double)>> eval_; | ||
|
||
public: | ||
// First constructor, takes parameters from IfcCurveSegment | ||
curve_segment_evaluator(double length_unit, IfcSchema::IfcCurve* curve, IfcSchema::IfcCurveMeasureSelect* st, IfcSchema::IfcCurveMeasureSelect* le) | ||
curve_segment_evaluator(double length_unit, segment_type_t segment_type, IfcSchema::IfcCurve* curve, IfcSchema::IfcCurveMeasureSelect* st, IfcSchema::IfcCurveMeasureSelect* le) | ||
: length_unit_(length_unit) | ||
, segment_type_(segment_type) | ||
, curve_(curve) | ||
{ | ||
// @todo in IFC4X3_ADD2 this needs to be length measure | ||
|
@@ -146,7 +131,7 @@ class curve_segment_evaluator { | |
// // transform point into clothoid's coodinate system | ||
// auto x = xl * cos(theta) - yl * sin(theta) + Cx; | ||
// auto y = xl * sin(theta) + yl * cos(theta) + Cy; | ||
// return Eigen::Vector3d(x, y, 0.0); | ||
// return Eigen::VectorXd(x, y, 0.0); | ||
// }; | ||
// } | ||
//#endif | ||
|
@@ -185,21 +170,26 @@ class curve_segment_evaluator { | |
auto Cy = C->as<IfcSchema::IfcCartesianPoint>()->Coordinates()[1]; | ||
|
||
eval_ = [L, Cx, Cy, theta, signX, fnX, signY, fnY](double u) { | ||
using boost::math::quadrature::trapezoidal; | ||
|
||
// integration limits, integrate from a to b | ||
// from 8.9.3.19.1, integration limits are 0.0 to u where u is a normalized parameter | ||
auto a = 0.0; | ||
auto b = fabs(u / L); | ||
|
||
auto n = 10; // use 10 steps in the numeric integration | ||
// @todo where to plug this in? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the 4th argument to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so. the docs say:
https://live.boost.org/doc/libs/1_68_0/libs/math/doc/html/math_toolkit/trapezoidal.html So it's not just a fixed increment, but some limit to the number of refinement iterations. So conceptually it'd be more like 2^10 (big question mark). That's why I was a bit afraid to plug it in, but the default is 12. So I think we can just keep the defaults. |
||
// auto n = 10; // use 10 steps in the numeric integration | ||
|
||
auto xl = signX(u)*integrate(a, b, n, fnX); | ||
auto yl = signY(u)*integrate(a, b, n, fnY); | ||
auto xl = signX(u)*trapezoidal(fnX, a, b); | ||
auto yl = signY(u)*trapezoidal(fnY, a, b); | ||
|
||
// transform point into clothoid's coodinate system | ||
auto x = xl * cos(theta) - yl * sin(theta) + Cx; | ||
auto y = xl * sin(theta) + yl * cos(theta) + Cy; | ||
return Eigen::Vector3d(x, y, 0.0); | ||
}; | ||
Eigen::VectorXd vec(3); | ||
vec << x, y, 0.0; | ||
return vec; | ||
}; | ||
} | ||
|
||
// Clothoid using numerical integration | ||
|
@@ -290,8 +280,10 @@ class curve_segment_evaluator { | |
// transform point into circle's coodinate system | ||
auto x = xl * cos(theta) - yl * sin(theta) + Cx; | ||
auto y = xl * sin(theta) + yl * cos(theta) + Cy; | ||
return Eigen::Vector3d(x, y, 0.0); | ||
}; | ||
Eigen::VectorXd vec; | ||
vec << x, y, 0.0; | ||
return vec; | ||
}; | ||
} | ||
|
||
void operator()(IfcSchema::IfcPolyline* pl) | ||
|
@@ -361,8 +353,10 @@ class curve_segment_evaluator { | |
|
||
auto [u_start, u_end, compare] = iter->first; | ||
auto [x,y] = (iter->second)(u - u_start); // (u - u_start) is distance from start of this segment of the polyline | ||
return Eigen::Vector3d(x, y, 0); | ||
}; | ||
Eigen::VectorXd vec; | ||
vec << x, y, 0; | ||
return vec; | ||
}; | ||
} | ||
|
||
void operator()(IfcSchema::IfcLine* l) { | ||
|
@@ -376,11 +370,27 @@ class curve_segment_evaluator { | |
auto dx = dr[0] / m; | ||
auto dy = dr[1] / m; | ||
|
||
eval_ = [px, py, dx, dy](double u) { | ||
auto x = px + u * dx; | ||
auto y = py + u * dy; | ||
return Eigen::Vector3d(x, y, 0); | ||
if (segment_type_ == ST_HORIZONTAL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The treatment of line assumes the line is either in the xy plane for horizontal or the sz plane for vertical. I don't think the ifc specification requires this. CT 4.1.7.1.1.4 Alignment Geometry- Segments seems to require that the ParentCurve be 3D sz plane = developed elevation view of the alignment profile. s = distance along curve, z = elevation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks. That's exactly what I was beginning to wonder earlier on. If we don't have any guidance on this in the spec it's really an omission that needs to be corrected. Luckily we also have the validation service where such constraints can be incrementally rolled out (cc @civilx64) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if such constraints are required but they would make implementation much easier and rational. Consider a line in 3D space. A single IfcLine entity could be used for both the horizontal and vertical definitions. The projection of the line onto the xy and sz planes defines the horizontal and vertical geometry. This is essentially a diagonal line through opposite corners of a cube. That's fairly easy to deal with. Now consider a clothoid in an arbitrary plane in 3D space. I really don't want to figure out its projections onto the xy and sz planes. Additionally, I doubt the resulting geometry is actually used in practice since roadway and rail design predates computers - our predecessors engineers likely didn't make these calculations be hand. |
||
|
||
eval_ = [px, py, dx, dy](double u) { | ||
auto x = px + u * dx; | ||
auto y = py + u * dy; | ||
Eigen::VectorXd vec; | ||
vec << x, y, 0; | ||
return vec; | ||
}; | ||
|
||
} else if (segment_type_ == ST_VERTICAL) { | ||
|
||
eval_ = [px, py, dx, dy](double u) { | ||
// @todo this can't be correct | ||
auto z = u * dy; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. auto z = py + u*dy; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks |
||
Eigen::VectorXd vec; | ||
vec << z; | ||
return vec; | ||
}; | ||
|
||
} | ||
} | ||
|
||
// Take the boost::type value from mpl::for_each and test it against our curve instance | ||
|
@@ -392,7 +402,7 @@ class curve_segment_evaluator { | |
} | ||
|
||
// Then, with function populated based on IfcCurve subtype, we can evaluate to points | ||
Eigen::Vector3d operator()(double u) { | ||
Eigen::VectorXd operator()(double u) { | ||
if (eval_) { | ||
return (*eval_)((u + start_) * length_unit_); | ||
} else { | ||
|
@@ -403,23 +413,65 @@ class curve_segment_evaluator { | |
double length() const { | ||
return length_; | ||
} | ||
|
||
const std::optional<std::function<Eigen::VectorXd(double)>>& evaluation_function() const { | ||
return eval_; | ||
} | ||
}; | ||
|
||
taxonomy::ptr mapping::map_impl(const IfcSchema::IfcCurveSegment* inst) { | ||
// @todo fixed number of segments or fixed interval? | ||
// @todo placement | ||
// @todo figure out what to do with the zero length segments at the end of compound curves | ||
|
||
static int NUM_SEGMENTS = 64; | ||
curve_segment_evaluator cse(length_unit_, inst->ParentCurve(), inst->SegmentStart(), inst->SegmentLength()); | ||
boost::mpl::for_each<curve_seg_types, boost::type<boost::mpl::_>>(std::ref(cse)); | ||
bool is_horizontal = false; | ||
bool is_vertical = false; | ||
bool is_cant = false; | ||
|
||
std::vector<taxonomy::point3::ptr> polygon; | ||
{ | ||
aggregate_of_instance::ptr segment_owners = inst->data().getInverse(&IfcSchema::IfcCompositeCurve::Class(), 0); | ||
if (segment_owners) { | ||
for (auto& cc : *segment_owners) { | ||
if (cc->as<IfcSchema::IfcSegmentedReferenceCurve>()) { | ||
is_cant = true; | ||
} else if (cc->as<IfcSchema::IfcGradientCurve>()) { | ||
is_vertical = true; | ||
} else { | ||
is_horizontal = true; | ||
} | ||
} | ||
} | ||
} | ||
|
||
if ((is_horizontal + is_vertical + is_cant) != 1) { | ||
// We have to choose the correct functor based on usage. We can't | ||
// support multiple, because we don't know the caller at this point. | ||
return nullptr; | ||
} | ||
|
||
auto segment_type = is_horizontal ? ST_HORIZONTAL : is_vertical ? ST_VERTICAL : ST_CANT; | ||
|
||
curve_segment_evaluator cse(length_unit_, segment_type, inst->ParentCurve(), inst->SegmentStart(), inst->SegmentLength()); | ||
boost::mpl::for_each<curve_seg_types, boost::type<boost::mpl::_>>(std::ref(cse)); | ||
|
||
auto fn = *cse.evaluation_function(); | ||
auto length = cse.length(); | ||
|
||
// @todo - for some reason this isn't working, the matrix gets all messed up | ||
//const auto& transformation_matrix = taxonomy::cast<taxonomy::matrix4>(map(inst->Placement()))->ccomponents(); | ||
auto transformation_matrix = taxonomy::cast<taxonomy::matrix4>(map(inst->Placement()))->ccomponents(); | ||
|
||
auto fn_transformed = [fn, transformation_matrix](double u) { | ||
return transformation_matrix * fn(u); | ||
}; | ||
|
||
// @todo it might be suboptimal that we no longer have the spans now | ||
auto pwf = taxonomy::make<taxonomy::piecewise_function>(); | ||
pwf->spans.push_back({ length, fn_transformed }); | ||
return pwf; | ||
|
||
/* | ||
static int NUM_SEGMENTS = 64; | ||
std::vector<taxonomy::point3::ptr> polygon; | ||
|
||
auto length = cse.length(); | ||
if (0.001 < fabs(length)) | ||
{ | ||
|
@@ -433,6 +485,7 @@ taxonomy::ptr mapping::map_impl(const IfcSchema::IfcCurveSegment* inst) { | |
} | ||
|
||
return polygon_from_points(polygon); | ||
*/ | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/******************************************************************************** | ||
* * | ||
* This file is part of IfcOpenShell. * | ||
* * | ||
* IfcOpenShell is free software: you can redistribute it and/or modify * | ||
* it under the terms of the Lesser GNU General Public License as published by * | ||
* the Free Software Foundation, either version 3.0 of the License, or * | ||
* (at your option) any later version. * | ||
* * | ||
* IfcOpenShell is distributed in the hope that it will be useful, * | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of * | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * | ||
* Lesser GNU General Public License for more details. * | ||
* * | ||
* You should have received a copy of the Lesser GNU General Public License * | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. * | ||
* * | ||
********************************************************************************/ | ||
|
||
#include "mapping.h" | ||
#define mapping POSTFIX_SCHEMA(mapping) | ||
using namespace ifcopenshell::geometry; | ||
|
||
#ifdef SCHEMA_HAS_IfcGradientCurve | ||
|
||
taxonomy::ptr mapping::map_impl(const IfcSchema::IfcGradientCurve* inst) { | ||
auto horizontal = taxonomy::cast<taxonomy::piecewise_function>(map(inst->BaseCurve())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will work on caching next. It's trivial, it's just a std::map<uint, taxonomy::item::ptr>. With again the caveat that we should be a bit careful considering immutability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the map function returns a shared pointer to a constant taxonomy item? std::shared_ptr The map function creates the taxonomy::item and all other places in the code are consumers of the taxonomy::item. Do consumers have a need to alter the item? Probably not. This would enforce immutability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are one or two cases that still need to be worked out with orientation on things like IfcOrientedEdge, where the consumer does indeed alter item (minimally). (In OpenCascade they have their own shared pointer / handle, but orientation is basically stored on the pointer, not in the data.) |
||
auto vertical = taxonomy::make<taxonomy::piecewise_function>(); | ||
|
||
auto segments = inst->Segments(); | ||
|
||
for (auto& segment : *segments) { | ||
if (segment->as<IfcSchema::IfcCurveSegment>()) { | ||
// @todo check that we don't get a mixture of implicit and explicit definitions | ||
auto crv = map(segment->as<IfcSchema::IfcCurveSegment>()); | ||
if (crv->kind() == taxonomy::PIECEWISE_FUNCTION) { | ||
auto seg = taxonomy::cast<taxonomy::piecewise_function>(crv); | ||
vertical->spans.insert(vertical->spans.end(), seg->spans.begin(), seg->spans.end()); | ||
} else { | ||
Logger::Error("Unsupported"); | ||
return nullptr; | ||
} | ||
} else { | ||
Logger::Error("Unsupported"); | ||
return nullptr; | ||
} | ||
} | ||
|
||
// @todo does this really make sense? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is typically how it's handled when there is a horizontal and vertical definition. This doesn't work for CT 4.1.7.1.1.4 Alignment Geometry- Segments because it seems to require that the ParentCurve be 3D. There isn't independent horizontal and vertical components for this CT. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are making another assumption here - sort of - maybe it's just a distinction we need to be mindful of. For CT Alignment Geometry Segment, the representation is on the IfcCurveSegment whereas the other CTs put the representation on the IfcAlignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Where do you see this constraint encoded? I think that would be an error. I briefly looked and IfcShapeRepresentation.RepresentationType=Segment seems to just select IfcCurveSegment without any constraints on the parent curve. IFC is a community where clear choices are seldomly made. So I think the idea is you do everything. Individual representations for the segments is the most likely thing missing from these aspects. But you're right in the sense that due to the IFC schema flexibility, you never really know. I've updated my diagram. dot code
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Perhaps I over stated my assumption. There are 4 CTs, the first 3 decompose the 3D space into two-2D representations that are combined to get a 3D result. I assumed the 4th CT, Segments, models the 3D space directly. The ParentCurve could be in a 2D horizontal xy plane, but this would be the Horizontal CT. Not sure if it make sense that ParentCurve be in a vertical sz plane without a horizontal component. I'm not sure what use case was imagined for the 4th CT but to me it looks like it would apply for as-built survey data which would be 3D points or segments There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I interpreted this differently. In my view the segment geometry is not really meant to be interesting in itself, but more to provide a more granular coupling between the business logic and the geometry. So the concept template just expresses that and doesn't really relate to a usecase. For surveying usecases it is possible to express an IfcAlignment representation simply as as 3d polyline. (Or another possible top-level representation item for alignment: https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcOffsetCurveByDistances.htm) |
||
auto composition = [horizontal, vertical](double u) { | ||
auto xy = horizontal->evaluate(u); | ||
auto z = vertical->evaluate(u); | ||
Eigen::VectorXd vec(3); | ||
vec << xy(0), xy(1), z(0); | ||
return vec; | ||
}; | ||
|
||
// @todo where do we get the startdistalong from @civilx64's code? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you get it from segment.start and segment.length? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When iterating segments I keep a private variable
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wasn't very clear, this was mostly a note to myself. This was about: On the business logic side we have three parameters that can be used to independently change the start point of the vertical wrt to the horizontal:
I'm also really confused about the placement altogether. You can introduce gradient by means of IfcLine.Dir but also with the placement X-axis. Which dimensionality is a vertical alignment curve to begin with? Is it a curve in the XY-plane with that maps The attribute description of Placement is some really cryptic prose:
|
||
std::array<taxonomy::piecewise_function::ptr, 2> both = { horizontal , vertical }; | ||
double min_length = std::numeric_limits<double>::infinity(); | ||
for (auto i = 0; i < 2; ++i) { | ||
double l = 0; | ||
for (auto& s : both[i]->spans) { | ||
l += s.first; | ||
} | ||
if (l < min_length) { | ||
min_length = l; | ||
} | ||
} | ||
|
||
auto pwf = taxonomy::make<taxonomy::piecewise_function>(); | ||
pwf->spans.push_back({ min_length, composition }); | ||
return pwf; | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -113,6 +113,9 @@ BIND(IfcEdge); | |||||||||||||||||
BIND(IfcEdgeLoop); | ||||||||||||||||||
BIND(IfcPolyline); | ||||||||||||||||||
BIND(IfcPolyLoop); | ||||||||||||||||||
#ifdef SCHEMA_HAS_IfcGradientCurve | ||||||||||||||||||
BIND(IfcGradientCurve); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aothms This is a little problematic. IfcGradientCurve is a IfcCompositeCurve so the item gets processed twice and the result for IfcCompositeCurve is returned. When processed as IfCompositeCurve the IfcCurveSegment are the vertical segments. I have some code in my driver program that maps a IfcGradientCurve but gets back the implicit_item for the vertical components.
Reversing the order of BIND(IfcGradientCurve) and BIND(IfcCompositeCurve) fixes the problem, but I don't think it is the best solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry. Yes, that could be addressed like this: https://github.com/IfcOpenShell/IfcOpenShell/pull/3827/files#r1347391406 It's something I introduced in the implementation of caching. I could no longer use the return statement anymore (because writing to cache is at the bottom of the function). So now it "falls through" to the other. That's why my suggestion of the addition of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aothms This is still a little problematic. If Is there a function similar to For Listing the BIND statements for the more derived types before the parent types solves the problem, but this seems like a brittle and problematic solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree, but in quite a few cases we do depend on matching also inherited types: IfcOpenShell/src/ifcgeom/mapping/mapping.i Lines 33 to 37 in 661c8bc
IfcOpenShell/src/ifcgeom/mapping/mapping.i Lines 103 to 105 in 661c8bc
On the other hand in many other cases it's also problematic because in the inheritance hierarchy of the newly added IFC geometry types, they don't seem to consider liskov-substitution in determining whether entities are supposed to be subtypes, but instead rather pragmatically try to reuse some attribute definitions. So maybe we need to enhance the BIND() macro with an option to selectively specify whether inheritance is considered to have the best of both worlds? We could check equality of |
||||||||||||||||||
#endif | ||||||||||||||||||
BIND(IfcCompositeCurve); | ||||||||||||||||||
BIND(IfcTrimmedCurve); | ||||||||||||||||||
BIND(IfcArbitraryOpenProfileDef); | ||||||||||||||||||
|
@@ -150,4 +153,4 @@ BIND(IfcStyledItem); // -> style | |||||||||||||||||
|
||||||||||||||||||
#ifdef SCHEMA_HAS_IfcCurveSegment | ||||||||||||||||||
BIND(IfcCurveSegment); | ||||||||||||||||||
#endif | ||||||||||||||||||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boost math also has constants like pi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. Thanks.