#2412 - User can't correctly save (or make a layout) to RDF/RXN reaction several products or with separate positioned molecules #2717
Conversation
| @@ -822,5 +824,234 @@ namespace indigo | |||
| float _d; | |||
| }; | |||
|
|
|||
| inline bool isPointInPolygon(const Vec2f& p, const std::vector<Vec2f>& poly) | |||
There was a problem hiding this comment.
I think we need more comments.
Also no need to calculate x if both vertices has x > px
So I suggest something like
inline bool isPointInPolygon(const Vec2f& p, const std::vector<Vec2f>& poly)
{
// Ray casting algorithm
bool in = false;
for (size_t i = 0, n = poly.size(); i < n; ++i)
{
size_t j = (i + 1) % n;
if (((poly[i].y > p.y) != (poly[j].y > p.y)) && // point y between vertices and
(((p.x > poly[i].x) && (p.x < poly[j].x)) || // both vertices are at right of point or edge is at right of point
(p.x < (poly[i].x + (p.y - poly[i].y) * (poly[j].x - poly[i].x) / (poly[j].y - poly[i].y))))) // py < (py-y0)*dx/dy
in = !in;
}
return in;
}| return result; | ||
| } | ||
|
|
||
| inline float convexPolygonArea(const std::vector<Vec2f>& poly) |
There was a problem hiding this comment.
Could you please add comment that this is "Shoelace formula - triangle form"
| return result; | ||
| } | ||
|
|
||
| inline bool isPointInConvexPolygon(const Vec2f& p, const std::vector<Vec2f>& poly) |
There was a problem hiding this comment.
Looks like this function computation is more complex tnan isPointInPolygon.
For convex polygon in most cases no edge crossing computation need - edge will be at left or at right from point.
Most complex case need cross calculation for one edge.
| return std::abs(area) * 0.5f; | ||
| } | ||
|
|
||
| inline bool isInside(const Vec2f& edgeStart, const Vec2f& edgeEnd, const Vec2f& point) |
There was a problem hiding this comment.
I think it should be named isNormalNegative, and comment about "check sign of normal to (edgeStart,edgeEnd),(edgeStart,point)" will be usefull
| @@ -822,5 +824,234 @@ namespace indigo | |||
| float _d; | |||
| }; | |||
|
|
|||
| inline bool isPointInPolygon(const Vec2f& p, const std::vector<Vec2f>& poly) | |||
| { | |||
| bool in = false; | |||
There was a problem hiding this comment.
What about next two corner cases?
There was a problem hiding this comment.
Same for edge:
|
|
||
| inline bool convexPolygonsIntersect(const std::vector<Vec2f>& poly1, const std::vector<Vec2f>& poly2) | ||
| { | ||
| auto project = [](const std::vector<Vec2f>& poly, const Vec2f& axis) { |
There was a problem hiding this comment.
Please add comment that this is "Separating axis collision detection"
| return {s.x + t * dx1, s.y + t * dy1}; | ||
| } | ||
|
|
||
| inline std::vector<Vec2f> convexClip(const std::vector<Vec2f>& subject, const std::vector<Vec2f>& clip) |
There was a problem hiding this comment.
Please add comment that this is "Sutherland–Hodgman algorithm"
|
|
||
| inline bool isInside(const Vec2f& edgeStart, const Vec2f& edgeEnd, const Vec2f& point) | ||
| { | ||
| return (edgeEnd.x - edgeStart.x) * (point.y - edgeStart.y) - (edgeEnd.y - edgeStart.y) * (point.x - edgeStart.x) <= 0; |
There was a problem hiding this comment.
I think it just edgeStart.relaticeCross(edgeEnd, point) < 0, is'n it?
| Vec2f dir = p2 - p1; | ||
| for (size_t i = 0, n = poly.size(); i < n; ++i) | ||
| { | ||
| Vec2f A = poly[i]; |
There was a problem hiding this comment.
I think it should be "const Vect2f&" or just "vect2f a=poly[i] - p1"
| @@ -4775,6 +4775,40 @@ void BaseMolecule::getBoundingBox(float font_size, LABEL_MODE label_mode, Rect2f | |||
| bbox = Rect2f(a, b); | |||
| } | |||
|
|
|||
| std::vector<Vec2f> BaseMolecule::getConvexHull(const Vec2f& min_box) const | |||
| { | |||
There was a problem hiding this comment.
Please add comment that this is "Andrew's monotone chain convex hull algorithm"
| { | ||
| float dist = computeConvexDistance(_components[i].second, _components[j].second); | ||
| auto& mdi = _mol_distances[i]; | ||
| auto it = std::lower_bound(mdi.sorted_distances.begin(), mdi.sorted_distances.end(), std::make_pair(j, dist), |
There was a problem hiding this comment.
Looks like sorted_distance used only here.
Do you really need this variable?
| for (int i = 0; i < static_cast<int>(_zones.size()); ++i) | ||
| { | ||
| auto& zone = _zones[i]; | ||
| std::pair<int, std::unordered_set<int>> cur_zone(i, {}); |
There was a problem hiding this comment.
look like this variable unused
| if (doesRayIntersectPolygon(tail_start, *c_it, hull1) && doesRayIntersectPolygon(tail_start, *c_it, hull2)) | ||
| return zone1 ? zone1 : current_zone; | ||
| } | ||
| } |
There was a problem hiding this comment.
Please remove this redundant braces or move break inside
| { | ||
| for (auto& section : cz.second) | ||
| { | ||
| if (!(_zones[cz.first].zone_type == ZoneType::EPathWay && section > 1) && it_oz2->second.count(section ^ 1)) |
There was a problem hiding this comment.
Could you please add comment to this check?
For me not clear what is verified here.
| return result; | ||
| } | ||
|
|
||
| std::optional<std::pair<int, int>> ReactionMultistepDetector::findMaxSpecialZone(size_t mol_idx, std::map<int, std::unordered_set<int>>& zones) |
There was a problem hiding this comment.
could you please add comment that this function search sections intersected with component hull and fill zones with these sections and return max containment section in pair<zone,section>
| _moleculeCount = (int)_components.size(); | ||
| } | ||
|
|
||
| std::optional<std::pair<int, int>> ReactionMultistepDetector::isMergeable(size_t mol_idx1, size_t mol_idx2, std::optional<std::pair<int, int>> current_zone) |
There was a problem hiding this comment.
Looks like it should be std::optional<std::pair<int, int>>& current_zone
|
|
||
| auto zone1 = findMaxSpecialZone(mol_idx1, other_zones1); | ||
| auto zone2 = findMaxSpecialZone(mol_idx2, other_zones2); | ||
| std::set_intersection(other_zones1.begin(), other_zones1.end(), other_zones2.begin(), other_zones2.end(), std::inserter(comm_zones, comm_zones.begin()), |
There was a problem hiding this comment.
I think intersection calc better move to line 573
| { | ||
| case ZoneType::EPlus: | ||
| if ((doesVerticalLineIntersectPolygon(coords[0].x, hull1) && doesVerticalLineIntersectPolygon(coords[0].x, hull2)) || | ||
| (doesHorizontalLineIntersectPolygon(coords[0].x, hull1) && doesHorizontalLineIntersectPolygon(coords[0].x, hull2))) |
There was a problem hiding this comment.
Looks like it should be coord[0].y as parameter of doesHorizontalLineIntersectPolygon
| { | ||
| for (int section1 : sections1) | ||
| { | ||
| if (zt == ZoneType::EPathWay && section1 > 1) // pathway has only 2 opposite agents sections |
There was a problem hiding this comment.
As you use section index in such way I think better add constants like pathway_sections_count, pathway_section_top, pathway_section_bottom, pathway_max_opposite_section_index for all types and use it is this function and in zone creating functions like
szd.zone_type = ZoneType::EPathWay;
szd.zone_sections.resize(pathway_sections_count);
szd.zone_sections[pathway_section_top] = top;
szd.zone_sections[pathway_section_bottom] = bottom;
szd.zone_sections[pathway_section_right] = right;And add comment in constant definition about this loigic
Second option is to store opposite indexes in something like std::map<ZoneType,std::map<std::size_t,std::size_t>> to make logic more clear
| auto c_it = coords.begin(); | ||
| const Vec2f& head = *c_it++; | ||
| const Vec2f& spine_beg = *c_it++; | ||
| const Vec2f& spine_end = *c_it++; |
There was a problem hiding this comment.
Why you use spine_end? AFAIK spine is vertical so X coordinates of spine_end and spine_beg are same.
Generic request
#1234 – issue name