From 7667e5b2ec86d70d6275fbd24baa1b99e24381d3 Mon Sep 17 00:00:00 2001 From: Hirek193 Date: Sat, 7 Nov 2026 22:11:08 +0100 Subject: [PATCH] fix: the bugs and vulnerabilities SonarCloud reports on PR #216 --- application/editormode.cpp | 10 ++- application/editormodecatenary.cpp | 2 +- application/editormodenmt.cpp | 12 +++- application/editormodeparallel.cpp | 17 ++++- application/editormodeprofile.cpp | 6 +- application/editormodeselftestjoin.cpp | 7 ++- application/editormodeselftestmonkey.cpp | 6 +- application/editormodeselftestrails.cpp | 75 +++++++++++++++-------- application/editormodeselftestterrain.cpp | 11 ++-- application/editormodeselftesttrack.cpp | 5 +- application/editormodesrk.cpp | 19 +++++- application/editormodeswitchplace.cpp | 6 +- application/editormodetrackux.cpp | 10 ++- audio/radiostream.cpp | 15 +++-- editor/editorAlignment.cpp | 10 +-- editor/editorClips.cpp | 2 + editor/editorSwitchCurve.cpp | 8 ++- model/Model3d.cpp | 8 +-- pr-reviews/PR-150.md | 25 ++++++++ pr-reviews/PR-159.md | 29 +++++++++ pr-reviews/PR-165.md | 28 +++++++++ pr-reviews/PR-167.md | 22 +++++++ pr-reviews/PR-172.md | 35 +++++++++++ pr-reviews/PR-187.md | 35 +++++++++++ pr-reviews/PR-224.md | 29 +++++++++ pr-reviews/PR-227.md | 15 +++++ pr-reviews/PR-228.md | 18 ++++++ pr-reviews/PR-230.md | 15 +++++ pr-reviews/PR-231.md | 20 ++++++ pr-reviews/PR-234.md | 20 ++++++ pr-reviews/PR-239.md | 23 +++++++ pr-reviews/PR-240.md | 25 ++++++++ pr-reviews/PR-241.md | 29 +++++++++ pr-reviews/PR-243.md | 29 +++++++++ pr-reviews/PR-259.md | 21 +++++++ pr-reviews/PR-260.md | 20 ++++++ pr-reviews/PR-261.md | 24 ++++++++ pr-reviews/PR-262.md | 18 ++++++ pr-reviews/PR-263.md | 35 +++++++++++ pr-reviews/PR-264.md | 27 ++++++++ pr-reviews/PR-265.md | 32 ++++++++++ pr-reviews/README.md | 46 ++++++++++++++ rendering/common/windscreenwater.cpp | 3 +- rendering/opengl46/opengl46renderer.cpp | 9 ++- rendering/vulkan14/vulkan14cabprobe.cpp | 12 +++- rendering/vulkan14/vulkan14commands.cpp | 5 ++ rendering/vulkan14/vulkan14grass.cpp | 4 +- rendering/vulkan14/vulkan14indirect.cpp | 3 +- rendering/vulkan14/vulkan14lamps.cpp | 3 +- rendering/vulkan14/vulkan14memory.cpp | 29 ++++++--- rendering/vulkan14/vulkan14shaders.cpp | 9 ++- shaders/compile_spirv.py | 14 ++--- tests/unit/test_alignment.cpp | 8 ++- tests/unit/test_switch_curve.cpp | 9 ++- tools/editor/usage_headers.py | 4 +- utilities/utilities.cpp | 6 +- utilities/utilities.h | 2 +- utilities/workerpool.cpp | 2 +- world/RailCrossings.cpp | 20 +++--- world/RoadPoint.cpp | 2 +- world/Segment.cpp | 16 +++-- world/catenary_cantilever.cpp | 4 +- world/catenary_scene.cpp | 6 +- world/catenary_solver.cpp | 5 +- world/catenary_span.cpp | 7 ++- 65 files changed, 910 insertions(+), 121 deletions(-) create mode 100644 pr-reviews/PR-150.md create mode 100644 pr-reviews/PR-159.md create mode 100644 pr-reviews/PR-165.md create mode 100644 pr-reviews/PR-167.md create mode 100644 pr-reviews/PR-172.md create mode 100644 pr-reviews/PR-187.md create mode 100644 pr-reviews/PR-224.md create mode 100644 pr-reviews/PR-227.md create mode 100644 pr-reviews/PR-228.md create mode 100644 pr-reviews/PR-230.md create mode 100644 pr-reviews/PR-231.md create mode 100644 pr-reviews/PR-234.md create mode 100644 pr-reviews/PR-239.md create mode 100644 pr-reviews/PR-240.md create mode 100644 pr-reviews/PR-241.md create mode 100644 pr-reviews/PR-243.md create mode 100644 pr-reviews/PR-259.md create mode 100644 pr-reviews/PR-260.md create mode 100644 pr-reviews/PR-261.md create mode 100644 pr-reviews/PR-262.md create mode 100644 pr-reviews/PR-263.md create mode 100644 pr-reviews/PR-264.md create mode 100644 pr-reviews/PR-265.md create mode 100644 pr-reviews/README.md diff --git a/application/editormode.cpp b/application/editormode.cpp index 0ce777d1e..e29cb6b54 100644 --- a/application/editormode.cpp +++ b/application/editormode.cpp @@ -5658,8 +5658,14 @@ void tutorial_signal_sketch(ImDrawList &List, ImVec2 const Low, ImVec2 const Hig // the track seen from above: sleepers and the two rails auto const middle{at(0.0f, 0.5f).y}; auto const gauge{height * 0.07f}; - for (float x = 0.02f; x < 1.0f; x += 0.025f) - List.AddLine(ImVec2(at(x, 0.0f).x, middle - gauge * 1.6f), ImVec2(at(x, 0.0f).x, middle + gauge * 1.6f), IM_COL32(110, 90, 70, 255), 3.0f); + { + float x{0.02f}; + while (x < 1.0f) + { + List.AddLine(ImVec2(at(x, 0.0f).x, middle - gauge * 1.6f), ImVec2(at(x, 0.0f).x, middle + gauge * 1.6f), IM_COL32(110, 90, 70, 255), 3.0f); + x += 0.025f; + } + } List.AddLine(ImVec2(Low.x, middle - gauge), ImVec2(High.x, middle - gauge), IM_COL32(200, 200, 205, 255), 2.0f); List.AddLine(ImVec2(Low.x, middle + gauge), ImVec2(High.x, middle + gauge), IM_COL32(200, 200, 205, 255), 2.0f); auto const line{ImGui::GetTextLineHeight()}; diff --git a/application/editormodecatenary.cpp b/application/editormodecatenary.cpp index 88530b788..5e8557d24 100644 --- a/application/editormodecatenary.cpp +++ b/application/editormodecatenary.cpp @@ -2379,7 +2379,7 @@ void catenary_editor::catenary_spread_create(std::size_t const Index) auto const *scheme{current != nullptr && catalog != nullptr ? catalog->scheme(current->card, current->execution) : nullptr}; auto const steadyonly{scheme != nullptr && scheme->roles.count("strut") == 0 && scheme->roles.count("stay") == 0}; auto const consolename{pole == nullptr ? std::string{} : steadyonly && false == pole->steady_console.empty() ? pole->steady_console : pole->console}; - auto const *console{false == consolename.empty() ? catalog->mount(consolename) : nullptr}; + auto const *console{false == consolename.empty() && catalog != nullptr ? catalog->mount(consolename) : nullptr}; if (current == nullptr || found == nullptr || false == current->has_pole || false == current->umbrella.empty()) return; if (console == nullptr || false == console->console) diff --git a/application/editormodenmt.cpp b/application/editormodenmt.cpp index 9285c0630..d92837e35 100644 --- a/application/editormodenmt.cpp +++ b/application/editormodenmt.cpp @@ -2056,9 +2056,13 @@ std::size_t editor_mode::nmt_level_turntables() if (state.unfitted && grade_fitted(*track)) fitted.push_back(end.point.y); auto const away{geometry::plan_of(end.point - table.centre) / std::max(1e-6, geometry::plan_distance(end.point, table.centre))}; - for (auto out{kPitWall}; out <= kPitWall + 6.0; out += 1.0) + auto out{kPitWall}; + while (out <= kPitWall + 6.0) + { if (double height; m_nmt.height(end.point.x + away.x * out, end.point.z + away.y * out, height)) model.push_back(height + state.offset + track->rail_height()); + out += 1.0; + } } auto &heights{fitted.empty() ? model : fitted}; if (heights.empty()) @@ -2276,8 +2280,12 @@ bool editor_mode::nmt_fit_line(TTrack &Track, std::functiontable->name()}); - for (auto at{from}; at <= from + level + 1e-6; at += 1.0) + auto at{from}; + while (at <= from + level + 1e-6) + { options.bounds.push_back({at, height - kJoin, height + kJoin, false}); + at += 1.0; + } WriteLog(format("Editor: NMT line %s runs up to turntable %s level at %.2f m over %.1f m", Track.name().c_str(), table->table->name().c_str(), height, level), logtype::generic); } auto const in_pit = [&](glm::dvec3 const &Point) { diff --git a/application/editormodeparallel.cpp b/application/editormodeparallel.cpp index 4a35bdd27..31fd8b27b 100644 --- a/application/editormodeparallel.cpp +++ b/application/editormodeparallel.cpp @@ -71,10 +71,17 @@ bool side_picker(int &Side, double const Spacing) auto const right{bottom.x - pad * 2.0f}; auto const gauge{std::round(font * 0.36f)}; // a track as two rails, sleepers under the solid ones + auto const sleepers = [list, left, right, font, gauge](float const Y, ImU32 const Colour) { + auto x{left + font * 0.3f}; + while (x < right) + { + list->AddLine(ImVec2(x, Y - gauge * 0.95f), ImVec2(x, Y + gauge * 0.95f), (Colour & ~IM_COL32_A_MASK) | IM_COL32(0, 0, 0, 90), 2.0f); + x += std::round(font * 0.9f); + } + }; auto const track = [&](float const Y, ImU32 const Colour, bool const Dashed) { if (false == Dashed) - for (auto x{left + font * 0.3f}; x < right; x += std::round(font * 0.9f)) - list->AddLine(ImVec2(x, Y - gauge * 0.95f), ImVec2(x, Y + gauge * 0.95f), (Colour & ~IM_COL32_A_MASK) | IM_COL32(0, 0, 0, 90), 2.0f); + sleepers(Y, Colour); for (auto const rail : {Y - gauge * 0.5f, Y + gauge * 0.5f}) { if (false == Dashed) @@ -82,8 +89,12 @@ bool side_picker(int &Side, double const Spacing) list->AddLine(ImVec2(left, rail), ImVec2(right, rail), Colour, 1.5f); continue; } - for (auto x{left}; x < right; x += font) + auto x{left}; + while (x < right) + { list->AddLine(ImVec2(x, rail), ImVec2(std::min(right, x + font * 0.55f), rail), Colour, 1.0f); + x += font; + } } }; auto const line{ImGui::GetColorU32(ImGuiCol_Text, 0.85f)}; diff --git a/application/editormodeprofile.cpp b/application/editormodeprofile.cpp index cc11fa0bf..9db2485a2 100644 --- a/application/editormodeprofile.cpp +++ b/application/editormodeprofile.cpp @@ -2709,8 +2709,12 @@ void editor_mode::draw_profile_canvas(profile_view const &View, ImDrawList &Draw // breaks the grade over them Draw.PushClipRect(ImVec2(left, plottop), ImVec2(right, plotbottom), true); auto const height{plotbottom - plottop}; - for (auto x = left - height; x < right; x += 8.0f) + auto x = left - height; + while (x < right) + { Draw.AddLine(ImVec2(x, plotbottom), ImVec2(x + height, plottop), kShared, 1.5f); + x += 8.0f; + } Draw.PopClipRect(); Draw.AddLine(ImVec2(left, plottop), ImVec2(left, plotbottom), kSharedEdge, 1.0f); Draw.AddLine(ImVec2(right, plottop), ImVec2(right, plotbottom), kSharedEdge, 1.0f); diff --git a/application/editormodeselftestjoin.cpp b/application/editormodeselftestjoin.cpp index 7d8e205e7..ee6fef8d4 100644 --- a/application/editormodeselftestjoin.cpp +++ b/application/editormodeselftestjoin.cpp @@ -545,8 +545,13 @@ editor_mode::selftest_result editor_mode::selftest_join(std::string const &comma lay_point(truth.start); lay_switch_mode(); int count{0}; - for (auto chainage{std::max(0.0, from - 30.0)}; chainage <= to; chainage += spacing, ++count) + auto chainage{std::max(0.0, from - 30.0)}; + while (chainage <= to) + { lay_point(along(chainage, noise * (count % 3 == 0 ? 1.0 : count % 3 == 1 ? -0.6 : 0.2))); + chainage += spacing; + ++count; + } lay_point(along(to + 30.0, noise * 0.5)); lay_point(along(to + 60.0, -noise * 0.5)); lay_switch_mode(); diff --git a/application/editormodeselftestmonkey.cpp b/application/editormodeselftestmonkey.cpp index 374b27e48..e4a3d8881 100644 --- a/application/editormodeselftestmonkey.cpp +++ b/application/editormodeselftestmonkey.cpp @@ -47,7 +47,7 @@ editor_mode::selftest_result editor_mode::selftest_monkey(std::string const &com unsigned int seed{1}; int steps{100}; words >> seed >> steps; - std::mt19937 random{seed}; + std::mt19937 random{seed}; // NOSONAR: a seeded, repeatable self-test, not security-relevant auto const pick = [&](int const Count) { return std::uniform_int_distribution(0, Count - 1)(random); }; auto const within = [&](float const Low, float const High) { return std::uniform_real_distribution(Low, std::max(Low + 1.0f, High))(random); }; struct area @@ -220,7 +220,7 @@ editor_mode::selftest_result editor_mode::selftest_monkey(std::string const &com { unsigned int seed{1}; words >> seed; - std::mt19937 random{seed}; + std::mt19937 random{seed}; // NOSONAR: a seeded, repeatable self-test, not security-relevant std::vector tracks; for (auto *track : simulation::Paths.sequence()) if (track != nullptr && false == track->m_editorremoved && false == track->m_paths.empty()) @@ -258,7 +258,7 @@ editor_mode::selftest_result editor_mode::selftest_monkey(std::string const &com screen_projection const projection; auto const low{editor_ui::view_min()}; auto const high{editor_ui::view_max()}; - std::shuffle(tracks.begin(), tracks.end(), random); + std::shuffle(tracks.begin(), tracks.end(), random); // NOSONAR: a seeded, repeatable self-test, not security-relevant for (auto const *track : tracks) { ImVec2 at; diff --git a/application/editormodeselftestrails.cpp b/application/editormodeselftestrails.cpp index ee1e7b4f3..5c15e2af0 100644 --- a/application/editormodeselftestrails.cpp +++ b/application/editormodeselftestrails.cpp @@ -167,11 +167,13 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm if (segment == nullptr) continue; output << "axis " << track->name() << ' ' << path; - for (auto at{0.0}; at <= segment->GetLength() + 1e-6; at += 0.25) + auto at{0.0}; + while (at <= segment->GetLength() + 1e-6) { glm::dvec3 spot, across; segment->plan_frame(std::min(at, segment->GetLength()), spot, across); output << format(" %.4f %.4f", spot.x, spot.z); + at += 0.25; } output << "\n"; } @@ -379,8 +381,11 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm auto const lengthb{b->eType == tt_Switch ? b->SwitchExtension->Segments[pb]->GetLength() : b->Segment->GetLength()}; auto least{1e9}; auto where{0.0}; - for (auto at{0.0}; at <= length; at += 0.02) + auto next_at{0.0}; + while (next_at <= length) { + auto const at{next_at}; + next_at += 0.02; double d, l; track_rails::rail_offset(*b, pb, sb, track_rails::rail_point(*a, pa, sa, at, head_a), d, l); if (d < 0.0 || d > lengthb || l < -0.3 || l > 0.3) @@ -429,7 +434,9 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm continue; ++count; std::string hits; - for (auto at{from}; at <= to; at += 0.05) + auto at{from}; + while (at <= to) + { // the head of the check rail as drawn, 40 mm at the head of the rail of 73.5 for (auto const lateral : {-track_rails::check_flangeway, -track_rails::check_flangeway - 0.020 * size.head / 0.0735, -track_rails::check_flangeway - 0.040 * size.head / 0.0735}) { @@ -450,6 +457,8 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm hits += hit + format(" at %.2f m", at); } } + at += 0.05; + } if (false == hits.empty()) ++faults; WriteLog(format("SELFTEST check %s path %d side %+d: %.3f-%.3f m drawn of %.3f-%.3f m%s", name.c_str(), path, side, from, to, check.from, check.to, hits.empty() ? ", clear" : (", over" + hits + " FAULT").c_str())); @@ -553,8 +562,11 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm continue; ++count; std::string hits; - for (auto at{from}; at <= to; at += 0.02) + auto next_at{from}; + while (next_at <= to) { + auto const at{next_at}; + next_at += 0.02; auto const offset{knots::sample(offsets, at, 0.0)}; if (offset <= 0.002) continue; @@ -741,25 +753,31 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm where[Kind] = What; } }; // past the last knots of its lines, the lines planing on as they end - for (auto s{from}; s <= to + 1e-9; s += 0.02) { - auto const l{lines_at(part, s)}; - for (auto const *line : {l.extents, l.inner}) + auto s{from}; + while (s <= to + 1e-9) { - if (line->empty()) - continue; - auto const value{knots::sample(*line, s, 0.0)}; - auto const planing{line == l.extents ? value < ours.size.head - 1e-4 : value > 1e-4}; - // the lines have knots every 2 cm at most - if (planing && (s > line->back().x + 0.025 || s < line->front().x - 0.025)) - fault(2, format("%s at %.2f m, past its knots %.2f-%.2f m", line == l.extents ? "extents" : "inner", s, line->front().x, line->back().x)); + auto const l{lines_at(part, s)}; + for (auto const *line : {l.extents, l.inner}) + { + if (line->empty()) + continue; + auto const value{knots::sample(*line, s, 0.0)}; + auto const planing{line == l.extents ? value < ours.size.head - 1e-4 : value > 1e-4}; + // the lines have knots every 2 cm at most + if (planing && (s > line->back().x + 0.025 || s < line->front().x - 0.025)) + fault(2, format("%s at %.2f m, past its knots %.2f-%.2f m", line == l.extents ? "extents" : "inner", s, line->front().x, line->back().x)); + } + s += 0.02; } } // along the part: nothing of the head left, the head jumping in width auto start{from + 0.05}; auto end{to - 0.05}; if (part.before != nullptr && part.before->startkind == track_rails::rail_end::sharp) - for (auto s{from}; s < to; s += 0.02) + { + auto s{from}; + while (s < to) { double offset, low, high; head_of(ours, part, s, offset, low, high); @@ -768,17 +786,23 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm start = std::max(start, s + 0.04); break; } + s += 0.02; } + } std::vector stations, widths; - for (auto s{std::ceil(start / 0.02) * 0.02}; s <= end; s += 0.02) { - double offset, low, high; - head_of(ours, part, s, offset, low, high); - auto const width{std::max(0.0, high - low)}; - if (width < 0.001 && false == obtuse_near(part, s)) - fault(4, format("at %.2f m, %.1f mm left", s, width * 1000.0)); - stations.push_back(s); - widths.push_back(width); + auto s{std::ceil(start / 0.02) * 0.02}; + while (s <= end) + { + double offset, low, high; + head_of(ours, part, s, offset, low, high); + auto const width{std::max(0.0, high - low)}; + if (width < 0.001 && false == obtuse_near(part, s)) + fault(4, format("at %.2f m, %.1f mm left", s, width * 1000.0)); + stations.push_back(s); + widths.push_back(width); + s += 0.02; + } } // a step in width well past those around it: a head tapering off steeply at a wide angle aside for (std::size_t i = 1; i < widths.size(); ++i) @@ -797,8 +821,11 @@ editor_mode::selftest_result editor_mode::selftest_rails(std::string const &comm auto const &low{theirs.line->low}; auto const &high{theirs.line->high}; auto distance{-1e9}; - for (auto s{std::ceil(from / 0.02) * 0.02}; s <= to; s += 0.02) + auto next_s{std::ceil(from / 0.02) * 0.02}; + while (next_s <= to) { + auto const s{next_s}; + next_s += 0.02; double offset, lo, hi; head_of(ours, part, s, offset, lo, hi); if (hi - lo < 2.0 * tolerance) diff --git a/application/editormodeselftestterrain.cpp b/application/editormodeselftestterrain.cpp index 429d5e82f..297bbee16 100644 --- a/application/editormodeselftestterrain.cpp +++ b/application/editormodeselftestterrain.cpp @@ -367,11 +367,12 @@ editor_mode::selftest_result editor_mode::selftest_terrain(std::string const &co } // level off the rim within a centimetre double level{0.0}; - for (double off = 0.0; off <= length; off += 0.5) - if (std::abs(height_at(off) - points[segment_data::point::start].y) <= 0.01) - level = off; - else - break; + double off{0.0}; + while (off <= length && std::abs(height_at(off) - points[segment_data::point::start].y) <= 0.01) + { + level = off; + off += 0.5; + } WriteLog(format("SELFTEST turntable %s track %s: at the rim %.3f m, %+.2f per mille over its first 5 m, level within 1 cm of the table for %.1f m; out along it%s", table.table->name().c_str(), track->name().c_str(), rim.y, 1000.0 * (height_at(std::min(5.0, length)) - height_at(0.0)) / std::max(1e-6, std::min(5.0, length)), level, text.c_str())); } diff --git a/application/editormodeselftesttrack.cpp b/application/editormodeselftesttrack.cpp index 02f4e4481..70758ea96 100644 --- a/application/editormodeselftesttrack.cpp +++ b/application/editormodeselftesttrack.cpp @@ -305,12 +305,15 @@ editor_mode::selftest_result editor_mode::selftest_track(std::string const &comm to = std::max(to, e.chainage + e.length); } int count{0}; - for (auto chainage{std::max(0.0, from - 20.0)}; chainage <= std::min(result.length, to + 20.0); chainage += spacing, ++count) + auto chainage{std::max(0.0, from - 20.0)}; + while (chainage <= std::min(result.length, to + 20.0)) { auto const station{alignment::evaluate(result, chainage)}; glm::dvec3 const across{-station.direction.z, 0.0, station.direction.x}; auto const shift{noise * (count % 3 == 0 ? 1.0 : count % 3 == 1 ? -0.6 : 0.2)}; m_route.trace.points.push_back(station.position + glm::normalize(across) * shift); + chainage += spacing; + ++count; } auto const &middle{m_route.trace.points.empty() ? glm::dvec3{0.0} : m_route.trace.points[m_route.trace.points.size() / 2]}; WriteLog(format("SELFTEST tracesample: %zu points over %.1f m, the middle one at %.3f %.3f", m_route.trace.points.size(), to - from, middle.x, middle.z)); diff --git a/application/editormodesrk.cpp b/application/editormodesrk.cpp index da2901735..566b30bdc 100644 --- a/application/editormodesrk.cpp +++ b/application/editormodesrk.cpp @@ -3178,12 +3178,22 @@ void srk_tool::set_lines(block_edit const *Edit) auto const reach{paired ? (end.distance + ends[line.partner].distance) * 0.5 : end.distance}; auto const other{paired ? braking(line.partner) * (aspects[line.partner] == 4 ? 0.5 : 1.0) : least}; auto const sections{std::max(1.0, std::floor(reach / std::max(least, other)))}; - for (auto k{1.0}; k < sections; k += 1.0) + auto k{1.0}; + while (k < sections) + { places.push_back(end.distance * k / sections); + k += 1.0; + } } else - for (auto distance{least * kBlockShare}; distance <= end.distance - 100.0; distance += least * kBlockShare) + { + auto distance{least * kBlockShare}; + while (distance <= end.distance - 100.0) + { places.push_back(distance); + distance += least * kBlockShare; + } + } for (auto const distance : places) add_wrong(plan(distance, station::block_number(km_on(index, distance), line.number, rising(index)), -1, false)); } @@ -4371,8 +4381,11 @@ void srk_tool::render_canvas(float const Height) auto const low{kilometrage ? std::min(km_from, km_to) * 1000.0 : first}; auto const high{kilometrage ? std::max(km_from, km_to) * 1000.0 : last}; auto shown{std::numeric_limits::lowest()}; - for (auto mark{std::ceil(low / step) * step}; mark <= high; mark += step) + auto next_mark{std::ceil(low / step) * step}; + while (next_mark <= high) { + auto const mark{next_mark}; + next_mark += step; auto const x{x_of(kilometrage ? first + (mark / 1000.0 - km_from) / (km_to - km_from) * (last - first) : mark)}; auto const label{kilometrage ? format("km %.3f", mark / 1000.0) : format("%.0f m", mark)}; if (std::abs(x - shown) < ImGui::CalcTextSize(label.c_str()).x + 12.0f) diff --git a/application/editormodeswitchplace.cpp b/application/editormodeswitchplace.cpp index b55d27f02..035d4dda4 100644 --- a/application/editormodeswitchplace.cpp +++ b/application/editormodeswitchplace.cpp @@ -2185,8 +2185,12 @@ bool editor_mode::plan_curved_slip(editor_track::switch_template const &Shape, i Plan.snapped.clear(); // bent with a curve as sharp as its slip curves, the outer one would turn the other way double sharpest{0.0}; - for (auto s{middle - half}; s <= middle + half + 1e-9; s += 0.5) + auto s{middle - half}; + while (s <= middle + half + 1e-9) + { sharpest = std::max(sharpest, std::abs(switch_curve::curvature_at(points, s))); + s += 0.5; + } if (sharpest * curve.radius > 1.0 - 1e-6) { Plan.problem = format(STR_C("The curve of R %.0f m is as sharp as the slip curves of R %.0f m or sharper, the slip isn't bent into it"), 1.0 / sharpest, curve.radius); diff --git a/application/editormodetrackux.cpp b/application/editormodetrackux.cpp index c9c76aee7..ba0279f0f 100644 --- a/application/editormodetrackux.cpp +++ b/application/editormodetrackux.cpp @@ -467,13 +467,19 @@ void editor_mode::draw_track_labels() const std::vector> taken; auto const tracks_under = [&](ImVec2 const &From, ImVec2 const &To) { int count{0}; - for (auto y = From.y; y <= To.y + cell * 0.5f; y += cell) - for (auto x = From.x; x <= To.x + cell * 0.5f; x += cell) + auto y = From.y; + while (y <= To.y + cell * 0.5f) + { + auto x = From.x; + while (x <= To.x + cell * 0.5f) { int column, row; if (cell_of(std::min(x, To.x), std::min(y, To.y), column, row)) count += covered[static_cast(row) * columns + column]; + x += cell; } + y += cell; + } return count; }; float constexpr kRings[]{0.0f, 26.0f, 42.0f, 62.0f, 86.0f, 116.0f, 150.0f}; diff --git a/audio/radiostream.cpp b/audio/radiostream.cpp index ead876d27..6d231265a 100644 --- a/audio/radiostream.cpp +++ b/audio/radiostream.cpp @@ -85,7 +85,7 @@ std::string parse_address(std::string const &Url, stream_address &Address) { return "encrypted (https) streams aren't supported"; } - if (starts_with_nocase(url, "http://")) + if (starts_with_nocase(url, "http://")) // NOSONAR: internet radio streams come over plain http, which is all the player reads { url.erase(0, 7); } @@ -152,7 +152,7 @@ std::string resolve_redirect(std::string const &Location, stream_address const & { return "http:" + Location; } - auto const root{"http://" + Origin.authority()}; + auto const root{"http://" + Origin.authority()}; // NOSONAR: internet radio streams come over plain http, which is all the player reads if (false == Location.empty() && Location.front() == '/') { return root + Location; @@ -183,7 +183,7 @@ std::string parse_playlist(std::string const &Playlist) line = trim(line.substr(separator + 1)); } } - if (starts_with_nocase(line, "http://") || starts_with_nocase(line, "https://")) + if (starts_with_nocase(line, "http://") || starts_with_nocase(line, "https://")) // NOSONAR: internet radio streams come over plain http, which is all the player reads { return line; } @@ -730,7 +730,14 @@ radio_session::result radio_session::receive_audio(http_response const &Response radio_stream::~radio_stream() { - close(); + try + { + close(); + } + catch (std::exception const &) + { + // out of memory while retiring the worker: its thread still stops and is joined by its own destructor below + } // NOTE: destructors of retired workers wait for their threads to finish } diff --git a/editor/editorAlignment.cpp b/editor/editorAlignment.cpp index 525ab0152..8ca2d7456 100644 --- a/editor/editorAlignment.cpp +++ b/editor/editorAlignment.cpp @@ -1980,14 +1980,16 @@ std::optional lay_on(lay_request const &Request) auto const step{std::max(0.5, radius * glm::radians(2.0))}; double low{0.0}; double high{-1.0}; - for (auto arc{step}; arc < 2.0 * kPi * radius; arc += step) + auto tried{step}; + while (tried < 2.0 * kPi * radius) { - if (miss(arc)[0] <= 0.0) + if (miss(tried)[0] <= 0.0) { - high = arc; + high = tried; break; } - low = arc; + low = tried; + tried += step; } if (high < 0.0) return std::nullopt; diff --git a/editor/editorClips.cpp b/editor/editorClips.cpp index a1ef212c7..7e7b4be74 100644 --- a/editor/editorClips.cpp +++ b/editor/editorClips.cpp @@ -100,6 +100,8 @@ std::uint64_t clip::texture(double const Seconds) long long total{0}; for (auto const delay : frames.delays) total += delay; + if (total <= 0) + total = 1; // no frame with a delay: the clip stands still auto time{static_cast(std::floor(Seconds * 1000.0)) % total}; int index{0}; while (index + 1 < static_cast(frames.delays.size()) && time >= frames.delays[index]) diff --git a/editor/editorSwitchCurve.cpp b/editor/editorSwitchCurve.cpp index 3a8fee4b9..f78271b6d 100644 --- a/editor/editorSwitchCurve.cpp +++ b/editor/editorSwitchCurve.cpp @@ -157,12 +157,14 @@ switch_report check(std::vector const &Main, std::vector> const &Tracks, std: auto const along = [&](std::vector const &Track) { span result; auto const margin{std::min(1.0, (Track.back().station - Track.front().station) * 0.25)}; - for (auto s{Track.front().station + margin}; s <= Track.back().station - margin + 1e-9; s += 0.5) + auto s{Track.front().station + margin}; + while (s <= Track.back().station - margin + 1e-9) { auto const curvature{sign * curvature_at(Track, s)}; result.low = std::min(result.low, curvature); result.high = std::max(result.high, curvature); result.sharpest = std::max(result.sharpest, std::abs(curvature)); + s += 0.5; } return result; }; diff --git a/model/Model3d.cpp b/model/Model3d.cpp index a6b9f41bc..be208930b 100644 --- a/model/Model3d.cpp +++ b/model/Model3d.cpp @@ -1796,8 +1796,8 @@ void assign_lod_ranges(TSubModel *Root, std::string const &Filename) if (path.find("siec_trakcyjna/") == std::string::npos) return; } - static float const limits[]{0.f, 20.f, 40.f, 100.f, 300.f, 400.f}; - int const limitcount{6}; + static constexpr std::array limits{0.f, 20.f, 40.f, 100.f, 300.f, 400.f}; + constexpr std::size_t limitcount{limits.size()}; auto const radius{Root->m_boundingradius}; float const vanish{(radius > 0.f && radius < 0.5f) ? 400.f : 500.f}; float const farthest{15000.f}; // zakres nadawany przy wczytaniu, gdy model nie podał własnego @@ -1805,7 +1805,7 @@ void assign_lod_ranges(TSubModel *Root, std::string const &Filename) { TSubModel *submodel; std::string base; - int index; + std::size_t index; // the digit after _LOD, 0-9 }; std::vector phases; for (auto *submodel = Root; submodel != nullptr; submodel = submodel->NextGet()) @@ -1820,7 +1820,7 @@ void assign_lod_ranges(TSubModel *Root, std::string const &Filename) continue; if (submodel->fSquareMinDist != 0.f || submodel->fSquareMaxDist < farthest * farthest * 0.99f) continue; - phases.push_back({submodel, name.substr(0, mark), digit - '0'}); + phases.push_back({submodel, name.substr(0, mark), static_cast(digit - '0')}); } for (auto const &phase : phases) { diff --git a/pr-reviews/PR-150.md b/pr-reviews/PR-150.md new file mode 100644 index 000000000..4362b0e00 --- /dev/null +++ b/pr-reviews/PR-150.md @@ -0,0 +1,25 @@ +# Review PR #150 — refactor: split OnCommands from Train + +Autor: @jerrrrycho · base: `master` · 39 plików, +12675/−12526 · ostatnia aktualizacja 10.10 +https://github.com/MaSzyna-EU07/maszyna/pull/150 + +**Werdykt: Approve kierunku; Request changes — rebase i ustalenie kolejności z PR-ami dotykającymi `Train.cpp`** + +## Podsumowanie +`vehicle/Train.cpp` rozbity skryptem na `vehicle/train/`: `Train.cpp`, `TrainCabSetup.cpp`, `TrainCommands{Auxiliary,Brakes,Controls,Doors,Electrical,Lights,Misc,Radio}.cpp`, `TrainSounds.cpp`, `TrainState.cpp`. `Train.h` przeniesiony do `vehicle/train/Train.h`, include'y w ~20 plikach poprawione. Przy okazji poprawki Sonara. + +## Co sprawdziłem (mechanicznie) +Porównałem wszystkie linie usunięte z `Train.cpp` z liniami dodanymi w `vehicle/train/*` (bez białych znaków): +- ~12 500 linii przeniesionych 1:1; różni się ~160 linii usuniętych / ~300 dodanych, z czego większość to nagłówki licencji i `#include` w nowych plikach. ✔ +- Pozostałe różnice to połączenie zagnieżdżonych `if` (S1066), np. `else if (RELEASE) { if (type == push) … }` → `else if (RELEASE && type == push)`. Sprawdziłem, że w tych miejscach to ostatnia gałąź łańcucha, a przy `EpFuseSwitch(true) && dsbPneumaticSwitch` wywołanie z efektem ubocznym zostaje po lewej. Semantyka zachowana. ✔ + +## Uwagi blokujące +1. **Rebase.** Od 10.10 do `master` doszły pliki, które włączają stary nagłówek — np. `world/RoadPoint.cpp` ma `#include "vehicle/Train.h"`, a PR go nie zmienia → po merge build się wysypie. Po rebase: `git grep '"vehicle/Train.h"'` musi zwracać pusto. +2. **Kolejność z innymi PR-ami.** Ten PR koliduje z każdym otwartym PR-em, który zmienia `Train.cpp`/`Train.h`: #234, #259, #261, #165, #172 (i pośrednio `feat/gl46-migration`). Proponuję ustalić z maintainerami okno: albo najpierw zmergować te małe PR-y, a potem rebase tego (skrypt da się puścić ponownie), albo odwrotnie i poprosić autorów o rebase. Skoro podział robi skrypt, warto go dołączyć do PR-a (np. `tools/`), żeby dało się go powtórzyć na świeżym `master`. + +## Pozostałe uwagi +3. **Rozdzielić przeniesienie od poprawek.** Poprawki Sonara w tym samym commicie co przeniesienie utrudniają review (`git diff --color-moved` nie pokaże ich czysto). Najlepiej: commit 1 — czysty move (skrypt), commit 2 — poprawki. Przy merge **nie** squashować, żeby `git blame -C` dalej działał. +4. **Zakomentowany kod** (np. stary `ConverterSwitch` w `TrainCommandsElectrical.cpp`) został przerobiony z `//` na `/* … */`, a nie usunięty. Skoro i tak gaśniemy uwagi Sonara (S125), lepiej go usunąć. +5. **Nazewnictwo / folder** — opis mówi „TBD”. `vehicle/train/` jest OK; trzeba to tylko potwierdzić przed merge, bo późniejsza zmiana to kolejny duży konflikt. +6. **„Probably works™” / „Please test them as well”.** Testy jednostkowe nie linkują `TTrain`, więc regresje wyjdą tylko w grze. Proszę o krótką listę ręcznie przetestowanych komend (światła, pantografy, drzwi, hamulce, radio) albo o checklistę dla testerów. +7. `CMakeLists.txt` — sprawdzić, że nowe pliki są też w innych systemach budowania, jeśli jakieś są (np. projekt VS, skrypty `build-dispatch`). diff --git a/pr-reviews/PR-159.md b/pr-reviews/PR-159.md new file mode 100644 index 000000000..7ea442ddb --- /dev/null +++ b/pr-reviews/PR-159.md @@ -0,0 +1,29 @@ +# Review PR #159 — Trainset spawning, various Lua scripting changes + +Autor: @jakubg1 · base: `master` · 10 plików, +65/−38 · ostatnia aktualizacja 21.10 +https://github.com/MaSzyna-EU07/maszyna/pull/159 + +**Werdykt: Request changes — otwarte uwagi z rundy 21.10 nadal aktualne** + +## Podsumowanie +- `api.spawn_trainset(trainset)` w Lua — cały wpis `trainset … endtrainset` jak ze Startera. +- `state_serializer::insert_trainset_full()`. +- Rejestracja źródeł dymu i świateł pojazdu przeniesiona z `deserialize_node()` do `TDynamicObject::Init()` (`init_smoke_sources()`), więc działa też dla pojazdów wstawianych w trakcie symulacji. +- `insert_trainset()` dostaje parametr `Velocity`. + +## Co sprawdziłem +- Warunek świateł w `Init()` ma już poprawne nawiasy (`CategoryFlag == 1 && (front || rear)`) — uwaga @jerrrrycho z 04.09 zaadresowana. ✔ +- `Init()` zwraca `0.0` (porażka, pojazd jest potem kasowany) tylko przed `TurnOff()`, czyli przed nową rejestracją dymu/świateł — więc brak wiszących wskaźników w `Particles`/`Lights`. ✔ Wyjątek: pojazd z `Dim.L == 0` w `.fiz` przejdzie rejestrację, a potem zostanie usunięty jako „błąd”. Rzadkie, ale warto zabezpieczyć (rejestrować po sprawdzeniu długości albo wyrejestrować przy `delete`). + +## Uwagi blokujące (z poprzedniej rundy, wciąż w kodzie) +1. **Pętla w `insert_trainset_full()` nie zatrzymuje się na `endtrainset`.** Przykład @jerrrrycho: dwa składy w jednym stringu → oba wylądują na torze pierwszego. Do tego brak `endtrainset` nie daje błędu, a skład zostaje „w połowie”. Proponuję: czytać `node` aż do `endtrainset`; jeśli go nie ma — `ErrorLog` i usunąć już utworzone pojazdy (albo jasno udokumentować, że zostają). +2. **`lua_tostring()` może zwrócić `NULL`** (argument nie jest stringiem/liczbą) → `std::string` z `nullptr` = UB/crash. Zapowiedziany osobny PR dla wszystkich funkcji API jest OK, ale dla nowej funkcji to jedna linia: `luaL_checkstring(L, 1)`. +3. **Unikalne nazwy pojazdów.** Ponowne wywołanie z tym samym wpisem tworzy duplikaty nazw (`Vehicles.insert` zwróci `false`, log „duplicate vehicle name”), a pojazd i tak zostaje w świecie. Dla API skryptowego, które *z założenia* będzie wołane wielokrotnie, warto automatyczny sufiks albo jawny błąd i odrzucenie. + +## Pozostałe uwagi +4. **Parametr `Velocity` w `insert_trainset()`** — oba wywołania podają `0`, a `insert_trainset_full()` go nie używa. Jeśli to przygotowanie pod przyszłość — lepiej dodać go w PR, który go użyje (YAGNI). +5. **`init_smoke_sources() const`** — metoda `const`, która rejestruje `this` w globalnym menedżerze cząsteczek. Kompiluje się (jest przeciążenie `insert(…, TDynamicObject const*, …)`), ale semantycznie to efekt uboczny; bez `const` będzie czytelniej. +6. **Synchroniczne ładowanie** (zamrożenie przy spawnie) — zaakceptowane w dyskusji; warto dopisać to w dokumentacji funkcji Lua. +7. **Test dymu i świateł** — @jakubg1 sam prosił o sprawdzenie, że dym i światła pojawiają się poprawnie. Proszę o potwierdzenie (np. zrzut parowozu z dymem i składu z oświetleniem czoła) zarówno dla scenerii wczytanej, jak i składu ze `spawn_trainset`. +8. **Tytuł/opis**: „various Lua scripting changes”, a dochodzi jedna funkcja — opis do zaktualizowania. Historia commitów jest pomieszana (autor sam to pisze), więc squash przy merge. +9. Formatowanie: mieszane tabulatory/spacje w `DynObj.h` i `simulationstateserializer.cpp` (`if (!vehicle)` + tab). diff --git a/pr-reviews/PR-165.md b/pr-reviews/PR-165.md new file mode 100644 index 000000000..649dd5f19 --- /dev/null +++ b/pr-reviews/PR-165.md @@ -0,0 +1,28 @@ +# Review PR #165 — feat: optional in-game HUD overlay (speed, vehicle data, notch, signals, CA/SHP) + +Autor: @DJ-YASHITAKA · base: `master` · 11 plików, +2210/−11 · ostatnia aktualizacja 10.10 +https://github.com/MaSzyna-EU07/maszyna/pull/165 + +**Werdykt: Request changes — przypadkowe regresje w `Globals.h`, konflikty; proponuję zamienić na draft** + +## Podsumowanie +Przeciągalny HUD (prędkość z kolorowaniem limitu, łuki napięcia/prądu/pozycji/bocznika, nastawnik, sygnały, CA/SHP), tryby Standard/Custom/Off, okno personalizacji, konfiguracja w osobnym `hud.ini`. F1 przełącza tryby HUD, stary „Driving Aid” przechodzi pod Shift+F1. + +## Uwagi blokujące +1. **Niezwiązane zmiany w `utilities/Globals.h` — wyglądają na błąd przy merge:** + - `targetfps { 60.0f }` → `{ 0.0f }` — zmienia domyślny limit klatek wszystkim graczom, + - usunięty wpis `{"dynamicbrake", &uart_conf.dynamicenable}` z konfiguracji UART — psuje odczyt `eu07.ini` dla sprzętu UART (łamie zasadę „every existing file must still be read the way it was”). + Oba trzeba przywrócić. +2. **Konflikty** — GitHub pokazuje stan `dirty`. Od 10.10 na `master` weszło sporo zmian w `driveruilayer`/`Train.h`, więc rebase będzie konieczny. +3. **F1 zmienia znaczenie.** Gracze od lat mają pod F1 „Driving Aid”, a teraz dostaną cykl trybów HUD. Wg `CLAUDE.md` funkcja zmieniająca to, co symulator pokazuje, ma się dać wyłączyć w `eu07.ini`, a po wyłączeniu wszystko ma działać jak wcześniej — w tym klawisz F1. Proponuję: HUD domyślnie **wyłączony**, F1 bez zmian, osobny skrót/menu dla HUD (albo F1 przełącza HUD dopiero gdy `gui.hud.enabled yes`). +4. **Domyślny stan jest niejasny.** `hud_config::enabled { true }`, `mode { 0 }` (= Standard), a komentarz w `driveruilayer.cpp` mówi „fresh installs start OFF”. Proszę ustalić jedno i zgodnie z `CLAUDE.md` (pkt 3) — domyślnie wyłączony. + +## Pozostałe uwagi +5. **Gdzie leży `hud.ini`.** W dyskusji padło „game directory”. Gra z Steam/GOG bywa w `Program Files`, gdzie zapis się nie uda. `eu07.ini` jest czytane z profilu użytkownika (`user_config_path`) — `hud.ini` powinno iść tą samą ścieżką (czytanie z profilu, potem z folderu gry). Komentarz w `Globals.cpp` też wymaga aktualizacji, bo dyskusja w PR przewijała się między `eu07.ini` a `hud.ini`. +6. **`friend class hud_panel`** w `TTrain` i `TController` — HUD czyta prywatne pola. Lepiej dodać publiczne gettery `const` dla tego, co HUD potrzebuje (`fBlinkTimer` itd.), żeby UI nie wiązało się z wnętrzem klas. +7. **Konfiguracja w `Globals.h`**: kilkadziesiąt pól z tablicami C (`float arc_r[4]`, `int arc_col[4]`…) w globalnym `Global` — przenieść do własnego nagłówka HUD i użyć `std::array` (Sonar S5945). +8. **Font `fonts/lcdd.ttf`** — to asset z SVN. Fallback jest (dobrze), ale PR musi powiedzieć, co dodać do SVN i w jakiej kolejności. +9. **Znany błąd `VelNext == -1`** — do poprawy przed merge (wyświetlać „—” albo nic). +10. **Tłumaczenia** — nowe `STR_C(...)` wymagają wpisów w `lang/*.po` w SVN; @spot57 zwracał uwagę na jakość polskich tekstów. +11. **UX** — uwagi @spot57 i @maj00r (układ odporny na długość tłumaczeń, łuki zamiast pasków, bez dokładnego odwzorowania świateł) — warto odpisać, które przyjęte. +12. Autor sam pytał o przełączenie na draft — sugeruję to zrobić, dopóki pkt 1–4 nie są rozwiązane. diff --git a/pr-reviews/PR-167.md b/pr-reviews/PR-167.md new file mode 100644 index 000000000..a73ef96bc --- /dev/null +++ b/pr-reviews/PR-167.md @@ -0,0 +1,22 @@ +# Review PR #167 — whois: add lookahead mode for the timetable entry after the next stop + +Autor: @pwarszawski · base: `master` · 3 pliki, +42/−1 · Closes #164 +https://github.com/MaSzyna-EU07/maszyna/pull/167 + +**Werdykt: Approve z drobnymi uwagami** + +## Podsumowanie +Nowy tryb `whois +48` (`whois_name | whois_load`, bez `mode_alt`): do komórki pamięci trafia nazwa wpisu rozkładu o jedną pozycję za `StationIndex`, 0 i flaga postoju (1/0). Akcesor `TTrainParameters::TimeTableEntryAhead(Offset)` zwraca `TimeTable[0]` („nowhere”, `Ah == -1`) poza zakresem. + +## Co jest dobrze +- Kontynuacja sekwencji +0…+40, +56 zostaje wolne — zgodnie z wcześniejszym review. +- Uczciwie opisana zmiana dla +48 (dotąd działało jak +32) — wymagane przez zasady kompatybilności formatów. +- Akcesor z kontrolą zakresu, `TimeTable[0]` jako bezpieczna wartość. + +## Uwagi +1. **Kompatybilność +48.** Zmiana znaczenia istniejącej wartości flagi formalnie łamie regułę „every existing file must still be read the way it was”. Ryzyko jest małe, ale proponuję sprawdzić (grep po SVN `pctga`, katalog `scenery/`) czy żadna sceneria nie używa `whois` z wartością 48–55 — i dopisać wynik do opisu PR. +2. **Przeloty.** „passthroughs are not skipped” — dla dyżurnego rozróżniającego kierunki zwykle liczy się następna *stacja* rozgałęźna, a w rozkładzie po stacji często stoi kilka punktów przelotowych (posterunki, podg). Wtedy +48 zwróci posterunek, a nie kierunek. Warto to opisać w dokumentacji eventu albo przewidzieć w przyszłości wariant (+56?) z pomijaniem przelotów. +3. **Nazwa „none”.** Gdy brak maszynisty/właściciela, wpisuje się `"none"`, a poza końcem rozkładu `"nowhere"`. Dwie różne wartości dla „nic” to OK, ale proszę je udokumentować razem z trybem (wiki / dokumentacja eventów), bo skrypty scenerii będą je porównywać. +4. **Log.** `WriteLog` z konkatenacją — w nowym kodzie Sonar woli `std::format` (S6185). Treść logu można zachować identyczną. +5. **`TimeTableEntryAhead(int Offset)`** — komentarz w nagłówku mówi `Offset>=1`, ale funkcja przyjmuje też 0 i ujemne (zwraca odpowiednie wpisy lub `[0]`). Albo zaktualizować komentarz, albo dodać `assert`. +6. Na PR są 4 wcześniejsze komentarze review — upewnić się, że wszystkie są rozwiązane przed merge. diff --git a/pr-reviews/PR-172.md b/pr-reviews/PR-172.md new file mode 100644 index 000000000..538f12685 --- /dev/null +++ b/pr-reviews/PR-172.md @@ -0,0 +1,35 @@ +# Review PR #172 — UART MaSzyna Hardware Protocol v2 + +Autor: @Hirek193 · base: `master` · 34 pliki, +4860/−185 · ostatnia aktualizacja 10.10 +https://github.com/MaSzyna-EU07/maszyna/pull/172 + +**Werdykt: Request changes — konflikty, wątek startowany zawsze, brak testów kodeka** + +> Przegląd wyrywkowy: architektura, `hardware_manager`, `protocol_codec`, konfiguracja, zmiany w `Train.cpp`/`zmq_input`. `protocol_session.cpp` (1327 linii) tylko pobieżnie. + +## Podsumowanie +Nowy podsystem `hardware/`: transport (serial, przygotowany pod TCP), kodek ramek (COBS + CRC32C), sesja protokołu z heartbeatem, rejestry stanów/komend/symboli, subskrypcje, panel diagnostyczny w UI. Wspólny rejestr stanu `TrainState` używany też przez ZMQ. Nowe klucze `uart2*` w `eu07.ini`; stary UART działa bez zmian. + +## Co jest dobrze +- Przemyślany protokół z dokumentacją na wiki i biblioteką Arduino. +- Kodek ostrożny wobec danych z portu: `cobs_decode` sprawdza `index + runlength > Size`, `reader::require()` przed odczytem, limit rozmiaru ramki, CRC32C. ✔ +- Stary UART nietknięty, nowy włączany osobnymi kluczami — zgodne z zasadami kompatybilności `eu07.ini`. +- `get_engine_voltage()` wydzielone z `TTrain::Update()` bez zmiany logiki (sprawdziłem gałęzie: ED → `|Im·5|`, `Mn > 0 && |Im| > 0` → wzór, inaczej 0). ✔ +- ZMQ: nazwy pól wyjściowych zachowane w rejestrze (sprawdziłem m.in. `shp`, `springbrake_active`, `hv_current_3`, `time_millisecond_of_day`). ✔ + +## Uwagi blokujące +1. **Konflikty** — GitHub pokazuje stan `dirty`, ostatnia aktualizacja 10.10. Rebase na aktualny `master`. +2. **Wątek roboczy startuje zawsze.** `WITH_HARDWARE_PROTOCOL_V2` jest domyślnie `ON`, a `hardware_manager` uruchamia `std::thread` „even with no links configured” i co kilka sekund skanuje porty szeregowe. U każdego gracza, który nie ma żadnego sprzętu. Na Windows enumeracja COM-ów (np. porty Bluetooth) potrafi trwać, a dla niektórych sterowników jest widoczna. `CLAUDE.md`: nowa funkcja ma się dać wyłączyć i bez niej ma być jak dawniej. Proponuję startować wątek dopiero gdy jest `uart2` w `eu07.ini` albo gdy użytkownik otworzy panel diagnostyczny. +3. **Testy jednostkowe kodeka.** `protocol_codec` (COBS, CRC32C, `frame_decoder::feed`, reader/writer) to czyste funkcje — idealne do `tests/unit`. Proszę o testy: round-trip, ramka uszkodzona/ucięta, przekroczony limit rozmiaru, sklejone ramki w jednym `feed()`. To kod parsujący dane z zewnątrz, więc testy są tu szczególnie ważne. + +## Pozostałe uwagi +4. **Sonar / `CLAUDE.md`:** + - `std::thread` → `std::jthread` (S6168; tutaj wątek jest zawsze joinowany w destruktorze, więc zamiana bezpieczna), + - `std::lock_guard` → `std::scoped_lock` (S5997), + - `s_instance` — globalny wskaźnik na singleton (S5421); rozważyć przekazanie przez `drivermode_input`, + - `protocol_session.cpp` 1327 linii — prawie na pewno S3776/S134; proszę przejrzeć raport Sonara po rebase. +5. **`uart2ratelimit`** czyta 3 wartości bez walidacji (w odróżnieniu od pozostałych kluczy z `ParseOneClamped`) — wartości ujemne/zero mogą zablokować komunikację. +6. **Refaktor ZMQ w tym samym PR.** Przeniesienie pól ZMQ do wspólnego rejestru jest sensowne, ale zmienia działające API dla istniejących klientów. Proszę potwierdzić test z realnym klientem ZMQ (np. pulpit z kolejki SOPI) albo wydzielić do osobnego PR-a. +7. **`version_info.h.in` / gałąź w `VERSION_INFO`** — niezwiązane z UART, a zmienia linię wersji w logu. W CI (detached HEAD) `rev-parse --abbrev-ref HEAD` zwróci `HEAD`. Osobny mały PR. +8. Opis PR to tylko 2 linki — dopisać: co testowano (jaki sprzęt, ile linków naraz, reconnect po wypięciu kabla), wpływ na wydajność, i że stary `uart` działa obok (na różnych portach). +9. Komentarze po polsku bez polskich znaków (`napiecie`, `pradu`) — drobne, ale reszta kodu używa UTF-8. diff --git a/pr-reviews/PR-187.md b/pr-reviews/PR-187.md new file mode 100644 index 000000000..3ef178461 --- /dev/null +++ b/pr-reviews/PR-187.md @@ -0,0 +1,35 @@ +# Review PR #187 — AI braking: descents, approaches to a lower speed, and stopping 8-15 m short of a signal + +Autor: @pwarszawski · base: `master` (merge-base 66a940d, 21.10) · 6 plików, +319/−54 · Fixes #180 +https://github.com/MaSzyna-EU07/maszyna/pull/187 + +**Werdykt: Approve kierunku; Request changes — poprawki pod Sonar/GCC przed merge** + +> To jest podstawa stosu #187 → #263 → #264 → #265. Recenzje tamtych PR-ów dotyczą tylko ich przyrostu ponad ten PR. + +## Podsumowanie +1. `BrakeForceR()` bierze próg rapidu z rozrządu (`TBrake::GetRapidRatio`, NESt3/ESt4: 50 km/h zamiast stałych 40 km/h). +2. Utrzymywanie prędkości na spadku: horyzont przewidywania = połowa czasu reakcji hamulca (`brake_response_horizon`), ciągłe prawo regulacji zamiast skoku hamuj/jedź, rozdzielenie `AccDesired` (tylko hamulec) i `fAccDesiredGravity`. +3. Planowanie dojazdu z tablicy hamowania składu (`shaped_braking_demand` — bisekcja po stopniu hamowania), zatrzymanie 8–15 m przed semaforem, na wzniesieniu trzymanie składu hamulcem zespolonym. +4. Brak „puszczania” hamulca w strefie zbliżenia przy dojeździe do niższej prędkości; próba hamowania nie nadpisuje stopu/ograniczenia przed pociągiem. + +## Co jest dobrze +- Wyjątkowo rzetelna metodyka: 32 składy × 4 profile, czas rzeczywisty i x20, recheck na aktualnym master, testy na śliskiej szynie (0.3/0.5), konsultacja z youBy. Wyniki jednoznaczne (np. spadek: defekty 21/32 → 5/32, przejechania semafora 4 → 0). +- Zmiany predykcji (`GetRapidRatio`) zrobione wirtualnie w `TBrake` z domyślną dotychczasową aproksymacją — inne rozrządy bez zmian. +- Dobrze opisane znane ograniczenia (Rlmmp, 764, 413S/AL2) zamiast ich zamiatania. + +## Uwagi do poprawy (Sonar / GCC — wg `CLAUDE.md`) +1. **S107** — `shaped_braking_demand()` ma 8 parametrów (limit 7). Proponuję struct, np. `consist_brake_table { double vmax; std::span a0, a1; }` — przy okazji znikną surowe wskaźniki na tablice C (`double const *Brakea0`). +2. **S3358** — zagnieżdżony `?:` w `brake_response_horizon()` (`… ? BrakeDelay[isrelease ? 0 : 1] : BrakeDelay[isrelease ? 2 : 3]`). Ten sam wybór „opóźnienie napełniania wg G/P” pojawia się też w `IncBrake()` i `control_braking_force()` — warto helper `brake_fill_time(Vehicle)` / `brake_release_time(Vehicle)`. +3. **`&&` mieszane z `||` bez nawiasów** w `control_tractive_force()` (`velocity + 1.0 < VelNext || VelNext == 0.0 && … && …`) — GCC `-Wparentheses` i Sonar. Proszę dodać nawiasy (bez zmiany semantyki). +4. **S3608** — lambdy `fullbraking`/`travel` łapią `[&]`; jawne capture. +5. **S134/S3776** — `adjust_desired_speed_for_target_speed()` było już mocno zagnieżdżone, nowy blok „final approach” dokłada 2–3 poziomy. Wydzielenie gałęzi pociągowej (`Obey_train`) do osobnej metody, np. `final_approach_demand(vel, stoppingroom)`, i bloku „uphill coasting” (powtarza się 2×, różni się tylko offsetem) do helpera. +6. Funkcje `static` w pliku → anonimowy namespace (konwencja z `CLAUDE.md`). + +## Uwagi merytoryczne / pytania +7. **Przełącznik w `eu07.ini`?** `CLAUDE.md`: zmiana tego, co symulator robi z istniejącymi sceneriami, ma się dać wyłączyć. Tu można bronić tezy, że to poprawka błędów (przejechania semafora, piłowanie hamulcem), ale skala zmiany zachowania AI jest duża (np. zatrzymanie 8–15 m zamiast dotychczasowych marginesów zależnych od długości). Proszę maintainerów o świadomą decyzję: poprawka vs. funkcja z przełącznikiem. +8. **Koszt obliczeń.** `shaped_braking_demand` = do 30 iteracji bisekcji × 24 kroki całkowania, wołane do 2× na przebłysk AI. Przy scenerii z kilkudziesięcioma składami AI to wciąż mało, ale warto zmierzyć (profiler na l053_poludnie) albo zmniejszyć liczbę iteracji (tolerancja ~0.01 osiągana po ~9). +9. **Magiczne liczby**: 0.025 (próg „pod górę”), 0.15, 3.5, 50.0, 0.04, 15.0/7.5 — przynajmniej te używane w kilku miejscach jako `constexpr` z nazwą. +10. **Konflikt z #205**: wspomniane w komentarzu `int autoswitch;` — dobrze, że pominięte; potwierdzić kolejność merge. +11. **Komentarze w `Driver.h`** dla nowych pól są po polsku, a w `.cpp` po angielsku — drobne, ale lepiej jednolicie. +12. Opis PR bardzo długi; do merge commitu warto dać streszczenie + link, a szczegóły pomiarów zostawić w PR. diff --git a/pr-reviews/PR-224.md b/pr-reviews/PR-224.md new file mode 100644 index 000000000..d1ef2c286 --- /dev/null +++ b/pr-reviews/PR-224.md @@ -0,0 +1,29 @@ +# Review PR #224 — Migrate from float3d to glm (second approach) + +Autor: @docentYT · base: `master` · 22 pliki, +204/−639 +https://github.com/MaSzyna-EU07/maszyna/pull/224 + +**Werdykt: Request changes — build VR, opis, kilka miejsc do sprawdzenia** + +## Podsumowanie +Usunięcie `utilities/Float3d.{h,cpp}` (`float3`, `float4`, `float4x4`) na rzecz `glm::vec3/quat/mat4`. `TSubModel::fMatrix` z wskaźnika (w unii z `iMatrix`) staje się wartością `glm::mat4 {1.0f}`; o tym, czy macierz jest niejednostkowa, decyduje już tylko flaga `0x8000`. Serializacja `.e3d` przez nowe `utilities/serialize.h`. + +## Co sprawdziłem (OK) +- **Kolejność elementów macierzy w `.e3d`/`.t3d`:** `float4x4::operator()(x)[y]` = `e[4x+y]`, a `glm::make_mat4(e)` jest column-major, więc `matrix[x][y]` w nowym `readMatrix` i w `(de)serialize_float32/64` daje dokładnie ten sam układ bajtów. ✔ Pliki binarne pozostają zgodne w obie strony. +- `InitialRotateMatrix()` jest wierną kopią `float4x4::InitialRotate()`. ✔ +- Brak `new/delete` dla macierzy — uwagi z poprzedniej rundy (@jakubg1, @Hirek193) zaadresowane. ✔ +- `BinInit`: `m.size() > iMatrix` przy `iMatrix == -1` — `-1` konwertuje się do `size_t` max, więc warunek jest fałszywy i zostaje jednostkowa. Działa, ale patrz pkt 4. + +## Uwagi blokujące +1. **Build z OpenVR się nie skompiluje.** `vr/openvr_imp.cpp` (linie ~253 i ~349 na master) woła `TSubModel::ReplaceMatrix()`, które ten PR usuwa, a pliku `vr/` PR nie zmienia. CI buduje bez VR, więc tego nie złapie. Trzeba podmienić na przypisanie `fMatrix` **i ustawienie `iFlags |= 0x8000`** (patrz pkt 2). +2. **Flaga `0x8000` jest teraz jedynym źródłem prawdy.** Wcześniej renderer mnożył przez macierz, gdy wskaźnik nie był pusty. Teraz każde miejsce, które przypisuje `fMatrix` z zewnątrz (VR, ewentualne skrypty, przyszły kod), musi też ustawić flagę, inaczej macierz zostanie po cichu zignorowana. Proponuję setter `SetMatrix(glm::mat4 const&)`, który ustawia/kasuje flagę wg `== glm::mat4(1.0f)`, i prywatne `fMatrix`. +3. **Inne gałęzie i PR-y.** Usunięcie `Float3d.h` łamie każdy otwarty kod, który używa `float3` — np. #261 dodaje nowe `SetRotate(float3(1, 0, 0), …)`, a `feat/gl46-migration` (#216) pewnie też go używa. Proszę ustalić kolejność merge z maintainerami i zostawić komentarz w #261/#216. Sprawdzić też `betterRenderer/` (np. `IntelTAA.h` ma trafienia `float3`; to prawdopodobnie HLSL, ale build z `WITH_BETTER_RENDERER=ON` trzeba przepuścić). + +## Pozostałe uwagi +4. `if (m.size() > iMatrix)` — porównanie `size_t` z `int` (GCC `-Wsign-compare`, Sonar). Lepiej `if (iMatrix >= 0 && static_cast(iMatrix) < m.size())`. To też jawnie pokazuje intencję. Przy okazji: indeks poza zakresem w uszkodzonym pliku był wcześniej UB, teraz jest cicho ignorowany — warto zalogować `Bad model`. +5. **Zakomentowany kod** w `ParentMatrix()` z dopiskiem „It will not work, fMatrix is not a pointer anymore” — lepiej usunąć cały martwy blok (Sonar S125). +6. `TSubModel *Root … TODO: Fix missing delete` — jeśli to znany wyciek, lepiej osobne issue niż TODO w kodzie wprowadzone przy okazji. +7. **Pamięć:** każdy `TSubModel` ma teraz 64 B macierzy zamiast 8 B wskaźnika, a submodele binarne nie współdzielą już tablicy `Matrices`. Dla dużych scenerii (tysiące instancji modeli) warto zmierzyć zużycie RAM przed/po — opis wymienia to jako „potential downside”, przydałaby się liczba. +8. **Opis PR** jest bardzo skąpy jak na zmianę tej skali. Proszę dopisać: co testowano (np. wczytanie kilku scenerii z `.t3d` i `.e3d`, konwersja `convertmodels`, porównanie zrzutów ekranu, pantografy — tam jest dużo matematyki macierzowej), oraz że format plików się nie zmienia. +9. Formatowanie: `fMatrix =glm::mat4();` (brak spacji), mieszane wcięcia w `DynObj.cpp` (spacje vs tabulatory w zmienianych liniach). +10. `SafeNormalize` w `glmHelpers.h` — brak nowej linii na końcu pliku. diff --git a/pr-reviews/PR-227.md b/pr-reviews/PR-227.md new file mode 100644 index 000000000..169acce5d --- /dev/null +++ b/pr-reviews/PR-227.md @@ -0,0 +1,15 @@ +# Review PR #227 — simulationenvironment: Adjust turbidity values + +Autor: @dos1 · base: `master` · 1 plik, +6/−6 +https://github.com/MaSzyna-EU07/maszyna/pull/227 + +**Werdykt: Approve po uzupełnieniu opisu** + +## Podsumowanie +Zmiana mapowania zachmurzenia (`Global.Overcast`) na mętność atmosfery (`fTurbidity`): minimum 3 → 4 (bo 3 psuje model w nocy) i wyższe wartości w środkowym/wysokim zakresie. + +## Uwagi +1. **Pierwsze dwa progi są teraz identyczne** (`<= 0.10 ? 4` i `<= 0.20 ? 4`), podobnie `0.30/0.40 → 5`, `0.50/0.60 → 6` i `0.90/else → 9`. Drabinka 10 zagnieżdżonych `?:` z powtórzonymi wartościami to dobry moment, żeby ją uprościć — np. tablica `std::array` progów albo krótka funkcja. Przy okazji zniknie zgłoszenie Sonara S3358 (zagnieżdżony operator `?:`), które i tak dotyczy zmienianych linii. +2. **„Turbidity 3 breaks the model at night”** — warto w opisie dodać 1–2 zdania, *jak* się psuje (NaN? czarne/świecące niebo?) i ewentualnie zrzut ekranu. Jeśli problem jest w samym modelu Preetham (znane ograniczenie dla T < ~2–3 przy słońcu pod horyzontem), lepszym miejscem na poprawkę może być clamp w modelu nieba, a nie w mapowaniu pogody — wtedy wartość 3 zostałaby dostępna w dzień. +3. **Zmiana wyglądu istniejących scenerii** — przy bezchmurnej pogodzie niebo będzie bardziej mgliste niż dotąd. To uzasadniona poprawka, ale wg `CLAUDE.md` należy to wprost opisać w PR (najlepiej porównanie przed/po dla 0.0, 0.5 i 0.9 overcast, dzień i noc). +4. Do przetestowania razem z #228 (sRGB) i #230 (rozmiar Słońca/Księżyca) — wszystkie trzy zmieniają odbiór nieba. diff --git a/pr-reviews/PR-228.md b/pr-reviews/PR-228.md new file mode 100644 index 000000000..34c3e3d98 --- /dev/null +++ b/pr-reviews/PR-228.md @@ -0,0 +1,18 @@ +# Review PR #228 — utilities: XYZtoRGB: Use sRGB colorspace + +Autor: @dos1 · base: `master` · 1 plik, +2/−2 +https://github.com/MaSzyna-EU07/maszyna/pull/228 + +**Werdykt: Approve z drobnymi uwagami** + +## Podsumowanie +Zamiana macierzy odwrotnej XYZ→RGB z Adobe RGB na sRGB. Merytorycznie słuszne: reszta pipeline'u (tekstury, framebuffer sRGB, tonemapping) pracuje w sRGB/Rec.709, więc współrzędne Adobe RGB (szerszy gamut) wyświetlane jako sRGB dawały zbyt mało nasycone kolory nieba. + +## Uwagi +1. **Komentarz nad macierzą jest teraz mylący.** Nad aktywną linią nadal stoi `// M^-1 for Adobe RGB …`, a pod zakomentowaną `// m^-1 for sRGB:`. Proponuję odwrócić komentarze albo usunąć martwą macierz Adobe RGB całkiem — to jest dokładnie to, co Sonar oznacza jako „commented-out code” (S125). +2. **Widoczna zmiana wyglądu.** Wg `CLAUDE.md`: „A feature doesn't make existing sceneries look suddenly different after an update”. To jest poprawka błędu, więc mieści się w wyjątku, ale warto w opisie PR jasno napisać, co się zmienia wizualnie (kolor nieba/słońca), najlepiej ze zrzutami przed/po w kilku porach dnia. +3. **Renderer GL4.6.** Plik `utilities/color.h` zostaje po migracji, ale warto potwierdzić, czy `feat/gl46-migration` (#216) korzysta z tej samej funkcji i czy tam też ma iść ta zmiana (albo czy gałąź już ją ma). +4. Opis: literówka „depite” → „despite”. + +## Sugestia +Rozważyć połączenie/uzgodnienie z #227 (turbidity) — obie zmiany wpływają na ten sam model nieba i dobrze byłoby je przetestować razem. diff --git a/pr-reviews/PR-230.md b/pr-reviews/PR-230.md new file mode 100644 index 000000000..fc06e8690 --- /dev/null +++ b/pr-reviews/PR-230.md @@ -0,0 +1,15 @@ +# Review PR #230 — opengl33renderer: Fix up the sizes of Sun and Moon + +Autor: @dos1 · base: `master` · 1 plik, +5/−6 +https://github.com/MaSzyna-EU07/maszyna/pull/230 + +**Werdykt: Approve z prośbą o wskazanie odpowiednika w GL4.6** + +## Podsumowanie +Przywrócenie rozmiarów tarcz Słońca i Księżyca z legacy renderera (`lerp` zależny od wysokości nad horyzontem) w miejsce „fizycznych” 0.00463/0.00451, które przy typowym FoV wyglądają na zbyt małe. + +## Uwagi +1. **Renderer, który odchodzi.** Wg `CLAUDE.md` cała rodzina `rendering/opengl33*` zostanie usunięta przez #216, a zmiany na `master` w tym kodzie mają mieć w PR wskazany odpowiednik na `feat/gl46-migration` albo informację, że gałąź już go ma. Proszę dopisać, gdzie w `rendering/opengl46/` / `vulkan14/` liczony jest rozmiar tarcz i czy tam też trzeba to zmienić — inaczej po migracji regresja wróci. +2. **Komentarz TODO dla Słońca mówi „from the moon object”** — skopiowany z bloku Księżyca. Skoro linie i tak są dotykane, warto poprawić na „sun object”. +3. **Uzasadnienie „fails to take FoV into account”** — rozmiar kątowy *jest* niezależny od FoV; problem jest raczej percepcyjny (zbyt mała tarcza na ekranie przy 45°+ FoV). Sugeruję doprecyzować opis, żeby ktoś za rok nie „naprawił” tego z powrotem na wartości fizyczne. Rozważyć stałą nazwaną z komentarzem („celowo ~7× większe niż rzeczywiste ze względów percepcyjnych”). +4. Zrzut przed/po w opisie bardzo by się przydał (zmiana wyglądu istniejących scenerii — wg `CLAUDE.md` powinna być opisana). diff --git a/pr-reviews/PR-231.md b/pr-reviews/PR-231.md new file mode 100644 index 000000000..ef57ede81 --- /dev/null +++ b/pr-reviews/PR-231.md @@ -0,0 +1,20 @@ +# Review PR #231 — driveruilayer: Allow to toggle AI driver from the menu + +Autor: @dos1 · base: `master` · 1 plik, +13/−0 +https://github.com/MaSzyna-EU07/maszyna/pull/231 + +**Werdykt: Request changes (drobne)** + +## Podsumowanie +Nowe menu „Vehicle” z przełącznikiem „Autopilot”, wysyłające `aidriverenable` / `aidriverdisable` przez `m_relay`. Świetny pomysł dla nowych graczy, mała i czysta zmiana. + +## Uwagi +1. **Warunek widoczności jest bardziej restrykcyjny niż klawiatura.** Menu jest ukryte, gdy `Global.RealisticControlMode && !DebugModeFlag` — zawsze, także gdy gracz siedzi w kabinie. Tymczasem `command_relay::post()` (input/command.cpp ~l. 891) blokuje komendy pojazdu w trybie realistycznym tylko przy `FreeFlyModeFlag`. Czyli Shift+Q w kabinie działa, a pozycja menu znika. Proponuję warunek zgodny z relay'em: + ```cpp + bool const blocked = FreeFlyModeFlag && Global.RealisticControlMode && !DebugModeFlag; + ``` + i najlepiej `ImGui::BeginDisabled(blocked)` zamiast ukrywania — gracz widzi, że opcja istnieje. +2. **Skrót w menu.** `MenuItem` przyjmuje skrót — warto podać `"Shift+Q"` / `"Q"` (domyślne bindy z `driverkeyboardinput.cpp`), tak jak robią to pozycje w „Mode windows”. Uwaga: bindy są konfigurowalne, więc albo pobrać aktualny z mapy klawiszy, albo pominąć. +3. **Tłumaczenia.** `STR_C("Vehicle")` i `STR_C("Autopilot")` to nowe klucze — trzeba je dodać do `lang/*.po` w SVN (w repo jest tylko `editor_pl.po`). Proszę napisać w opisie PR, co dodać do SVN; bez tego gracze PL zobaczą angielskie napisy. Sprawdzić też, czy w innych miejscach UI tryb AI nie nazywa się inaczej (np. „AI driver”) — lepiej trzymać jedną nazwę. +4. **Multiplayer (#173)**: przy sterowaniu cudzym pojazdem/wspólną sesją przełączanie AI z menu może wymagać dodatkowych warunków — nie blokuje tego PR, ale warto mieć na uwadze. +5. Drobne: `auto const *driver` — `Mechanik` to wskaźnik nie-const, a `AIControllFlag` tylko czytamy, więc OK; Sonar może jednak zgłosić S6004 (zmienna używana tylko w warunku → `if (auto const *driver = …; driver && …)`). diff --git a/pr-reviews/PR-234.md b/pr-reviews/PR-234.md new file mode 100644 index 000000000..d8f669141 --- /dev/null +++ b/pr-reviews/PR-234.md @@ -0,0 +1,20 @@ +# Review PR #234 — Train: Handle 3-way light switches in headlighttoggle actions + +Autor: @dos1 · base: `master` · 1 plik (`vehicle/Train.cpp`), +60/−16 +https://github.com/MaSzyna-EU07/maszyna/pull/234 + +**Werdykt: Approve z uwagami (głównie refaktor i opis zmiany zachowania)** + +## Podsumowanie +`headlighttoggleleft/right` dla przełączników 3-pozycyjnych (brak `ggLeftEndLightButton`, jest `ggLeftLightButton`) cyklicznie przełącza: wył. → reflektor → czerwone → wył. Dzięki temu przełącznik da się obsłużyć myszą/VR. Poprzednia uwaga @jakubg1 (pojazdy bez przełączników) jest zaadresowana warunkiem `|| ggLeftLightButton.SubModel == nullptr`. + +## Uwagi +1. **Zmiana zachowania klawiatury.** `headlighttoggle*` to ta sama komenda, którą wywołuje klawisz. Na ~110 pojazdach z listy (303E, 4E, EN57, ET22, TEM2…) drugie wciśnięcie klawisza zapali teraz czerwone zamiast zgasić reflektor, a dopiero trzecie zgasi. To może zaskoczyć graczy (i scenariusze szkoleniowe). Proszę opisać to wprost w PR, albo rozważyć rozróżnienie źródła (mysz/VR vs klawiatura) — np. przez osobną komendę „cycle” dla kliknięcia na przełącznik. +2. **Duplikacja.** Blok lewy i prawy są identyczne poza maskami i przyciskami — razem ~60 linii kopii. Warto wyciągnąć helper w anonimowym namespace, np. + ```cpp + void toggle_headlight_3way(TTrain *Train, command_data const &Command, side Side); + ``` + albo przynajmniej `bool has_3way_switch(TGauge const &End, TGauge const &Main)`. Ta sama heurystyka („brak dedykowanego przełącznika = 3-way”) jest już w `OnCommand_redmarkerenableleft` — dobrze mieć ją w jednym miejscu. +3. **Złożoność.** Dodatkowe zagnieżdżenie `if` w `if (GLFW_PRESS)` może podbić S134/S3776 w Sonarze (QG przeszedł, ale warto zerknąć na szczegóły). Wczesny `return` dla `Command.action != GLFW_PRESS` uprości kod. +4. **Stan niespójny.** Jeśli oba bity (`headlight_left` i `redmarker_left`) są ustawione (np. przez inną komendę/skrypt), gałąź `else if` przejdzie na czerwone — OK, ale warto to świadomie skomentować. +5. Formatowanie: `} else {` w jednej linii odbiega od stylu reszty pliku (klamra w nowej linii). diff --git a/pr-reviews/PR-239.md b/pr-reviews/PR-239.md new file mode 100644 index 000000000..637e52111 --- /dev/null +++ b/pr-reviews/PR-239.md @@ -0,0 +1,23 @@ +# Review PR #239 — Refactor/mover long functions + +Autor: @jerrrrycho · base: `master` · 103 pliki, +13168/−3871 · 67 commitów +https://github.com/MaSzyna-EU07/maszyna/pull/239 + +**Werdykt: Request changes — rebase, opis, kolejność ze stosem AI/hamulców** + +> Przegląd wyrywkowy (rozmiar). Skupiłem się na strukturze zmian i na ryzyku dla innych PR-ów. + +## Podsumowanie +Rozbicie długich funkcji `TMoverParameters` (`RunCommand`, `TractionForce`, `UpdatePipePressure`, `IncMainCtrl`/`DecMainCtrl`, bateria…) i części `hamulce.cpp` na nazwane metody (`RunBrakeCommand`, `SeriesMotorTractionForce`, `UpdateMainValve`, `DieselElectricForce`…). Zmiana czysto strukturalna — pod S3776/S134. + +## Uwagi blokujące +1. **Diff zawiera infrastrukturę testów, która jest już na `master`.** `tests/unit/golden.cpp`, `test_controls.cpp`, `COVERAGE.md`, dane `tests/unit/data/fiz/*` itd. (~8 tys. linii) pokazują się jako „dodane”, bo trafiły na `master` innym PR-em (squash). Po rebase na aktualny `master` zostanie właściwy przyrost: `Mover.cpp`, `MOVER.h`, `hamulce.cpp` i ewentualne zmiany testów. Bez tego nie da się rzetelnie zrecenzować. +2. **Brak opisu.** Dla refaktoru tej skali proszę o: listę rozbitych funkcji, zapewnienie „bez zmian zachowania” i — kluczowe — **wynik testów golden** (`Unit tests: N/N passed`, bez regeneracji golden). Golden traces to tu jedyna siatka bezpieczeństwa; jeśli jakiś ślad trzeba było zregenerować, to znak zmiany zachowania i musi być opisany. +3. **Kolizja ze stosem #187 → #262 → #263 → #264 → #265** (wszystkie zmieniają `Mover.cpp::BrakeForceR`, `hamulce.h`, `Oerlikon_ESt`) oraz z #259/#260 (`MOVER.h`). Przeniesienie kodu do nowych funkcji sprawi, że tamte łatki się nie nałożą. Trzeba ustalić kolejność z maintainerami; moja propozycja: najpierw mniejsze PR-y z poprawkami zachowania, potem ten refaktor (refaktor łatwiej powtórzyć niż przepisać poprawki fizyki). + +## Pozostałe uwagi (z `MOVER.h`) +4. **Sygnatury `Run*Command(…, bool &OK)` zwracające też `bool`.** Dwa kanały wyniku (zwracany „obsłużono” i wyjściowy `OK`) są mylące. Rozważyć `std::optional` (brak = komenda nie z tej grupy) albo mały struct. +5. **Duplikacja `power()` const / non-const.** Można zaimplementować wersję non-const przez `const_cast` z wersji const (wzorzec Meyersa) albo zostawić jedną `const` + osobny setter. +6. **Nowe metody prywatne czy publiczne?** Większość pomocników (`UpdateMainValve`, `DieselElectricShuntForce`…) powinna być `private` — sprawdzić, że nie powiększają publicznego API `TMoverParameters`. +7. Parametry wyjściowe przez referencję (`double &dtrans, double &dmoment`, `double &tempUmax`) — przy okazji refaktoru warto zwracać struct (czytelność, Sonar). +8. 67 commitów — przy merge squash albo uporządkowanie historii. diff --git a/pr-reviews/PR-240.md b/pr-reviews/PR-240.md new file mode 100644 index 000000000..ead0ab495 --- /dev/null +++ b/pr-reviews/PR-240.md @@ -0,0 +1,25 @@ +# Review PR #240 — fix(gl46): editor cursor depth read in the biased depth pass (port of 7078edf) + +Autor: @miloszunio · base: `feat/gl46-migration` · 2 pliki, +86/−27 · Fixes #238 +https://github.com/MaSzyna-EU07/maszyna/pull/240 + +**Werdykt: Approve po teście w symulatorze i poprawkach pod Sonar** + +## Podsumowanie +Port poprawki z Vulkana do GL4.6: odczyt głębokości pod kursorem edytora w przebiegu głębi sceny (z `glPolygonOffset(-1,-1)`) dawał punkt ~1 piksel dalej — przy kącie ślizgowym nawet metry. Teraz: +- czytany jest blok 3×3 (PBO, a przy MSAA renderbuffer 3×3), +- bias jest kompensowany (mniejsza różnica sąsiadów na oś, większa z osi + `2^(ilogb(d)-23)`), +- unprojekcja używa widoku/projekcji/kamery z klatki, w której zlecono odczyt; punkt trzymany w world space (`dvec3`). + +## Co jest dobrze +- Poprawna analiza: przy reversed-Z ujemny offset oddala powierzchnię, więc dodanie `m + r` ją przywraca. +- Zapamiętanie widoku z klatki żądania usuwa drugi, niezależny błąd (ruch kamery między żądaniem a odczytem). +- Sprawdzenie wyniku `read_data()` zamiast sentinela `max()`. + +## Uwagi +1. **Brak testu w symulatorze** („Not run in the simulator yet”). Proszę przed merge sprawdzić scenariusz z opisu (SSR/AO on, perspektywa, kąt ślizgowy) oraz MSAA on/off i `gfx_skippipeline` — blit 3×3 z bufora MSAA to nowa ścieżka. +2. **Sonar S3608 / S1188.** Lambdy `depth_at` i `slope` łapią `[&]` i nie są przekazywane do algorytmu standardowego — Sonar to zgłosi. Proponuję jawne capture (`[&depths, this]`, `[&depth_at, pointdepth]`) albo funkcje wolne w anonimowym namespace (`depth_bias_slope(depths, size, centre)`). Zmniejszy to też złożoność `Update_Mouse_Position()`. +3. **Nagłówki (GCC).** `std::ldexp`, `std::ilogb` → ``, `std::array` → ``. Upewnić się, że są dołączone wprost (lista z `CLAUDE.md`). +4. **`m_mouseknown`** nigdy nie wraca do `false` (np. po zmianie scenerii / resecie renderera) — `Mouse_Position()` zwróci wtedy stary punkt z poprzedniego świata. Drobne, ale łatwo zresetować przy `Init()`/zmianie scenerii. +5. **Komentarz o biasie** jest bardzo długi (7 linii jednym ciągiem) — dobrze, że jest, ale czytelniej byłoby rozbić na punkty lub dać odnośnik do commita Vulkana 7078edf. +6. Opis PR wspomina „Built with `-fsyntax-only`” — warto poczekać na pełny build CI na gałęzi `feat/gl46-migration`. diff --git a/pr-reviews/PR-241.md b/pr-reviews/PR-241.md new file mode 100644 index 000000000..cf9052948 --- /dev/null +++ b/pr-reviews/PR-241.md @@ -0,0 +1,29 @@ +# Review PR #241 — feat(editor): area fill: Enter finishes an area, Esc drops the outline, a list of filled areas to change later + +Autor: @miloszunio · base: `feat/gl46-migration` · 7 plików, +751/−97 +https://github.com/MaSzyna-EU07/maszyna/pull/241 + +**Werdykt: Request changes — test w edytorze, undo przez regenerację, opis formatu** + +## Podsumowanie +Narzędzie wypełniania obszaru dostaje cykl życia: Enter kończy obszar, Esc porzuca kontur, lista wypełnionych obszarów (Zmień / Pokaż / Usuń), undo dla dodania/zmiany/usunięcia, ziarno `std::mt19937` przechowywane z obszarem. Trwałość w nowym `editor/editorFillStore.{hpp,cpp}` jako linie-komentarze w plikach warstw, modele nazwane `_`. + +## Co jest dobrze +- Realny problem UX (nie dało się zakończyć konturu) rozwiązany kompletnie, z listą i edycją. +- Format w liniach-komentarzach — symulator je pomija, więc sceneria z tą funkcją dalej czyta się w obecnym wydaniu (zgodnie z `CLAUDE.md`). +- `fill_store` wzorowany na istniejącym `switch_store` — spójna architektura. + +## Uwagi blokujące +1. **Nie uruchomione w edytorze** („Built with -fsyntax-only… Not run in the editor yet”). Przy tej skali zmian (zapis do pliku, usuwanie modeli po nazwie) proszę o przejście scenariusza z opisu, najlepiej skryptem `EU07_EDITOR_SELFTEST` na kopii scenerii: kontur → Enter → Zmień → przesuń punkt → Enter → Usuń → Undo → zapis → wczytanie → Zmień. +2. **Opis i kod mówią o różnych znacznikach.** Opis PR: `//$f`; kod (`kMark`, `editormarks`, komentarz w `.hpp`): `//$w`. Proszę ujednolicić opis i upewnić się, że `//$w` nie koliduje z niczym w Rainsted/StarterNG ani w innych narzędziach piszących do scenerii. +3. **Undo zmiany/usunięcia odtwarza modele z ziarna.** `std::uniform_real_distribution` jest zależne od implementacji biblioteki standardowej (MSVC ≠ libstdc++), a lista szablonów z banku węzłów może się zmienić między sesjami. W tej samej sesji i tym samym buildzie to zadziała, ale nie jest to „te same obiekty” w ogólności (`CLAUDE.md`: nie polegać na zachowaniu nieprzenośnym). Bezpieczniej: przed usunięciem zrobić snapshot usuwanych węzłów (`export_as_text`) i przy undo wstawić dokładnie je. Jeśli zostaje regeneracja — opisać ograniczenie w komentarzu. +4. **Identyfikacja modeli po nazwie `fillN_k`.** `owns()` uzna każdy węzeł o nazwie `fill1_7` za część obszaru `fill1` — także ręcznie nazwany przez autora scenerii. „Zmień”/„Usuń” go skasuje. `next_id()` sprawdza tylko `fillN_0`. Rozważyć mniej kolizyjny prefiks (np. `$fill1_7`) albo zapis listy nazw modeli w `//$w`. + +## Uwagi (Sonar / styl) +5. `std::string seed, density, spacing, …` w jednej deklaracji — S1659. +6. `erase(std::remove_if(...))` → `std::erase_if` (S6165); `std::find_if(begin, end, …)` → `std::ranges::find_if` (S6197). +7. `g_areas`, `g_loaded` — niestałe zmienne globalne (S5421). Jeśli `switch_store` robi tak samo, to OK dla spójności, ale lepiej trzymać stan w obiekcie należącym do `editor_mode`. +8. Magiczne `100000` (limit punktów konturu) → nazwana stała. + +## Pytanie o gałąź +9. Funkcja edytora trafia do `feat/gl46-migration` (#216, draft, merge po 26.11). Czy to celowe (narzędzie istnieje tylko na tej gałęzi)? Jeśli kod narzędzia jest też na `master`, `CLAUDE.md` każe robić funkcje na własnej gałęzi od `master`. diff --git a/pr-reviews/PR-243.md b/pr-reviews/PR-243.md new file mode 100644 index 000000000..45cf45495 --- /dev/null +++ b/pr-reviews/PR-243.md @@ -0,0 +1,29 @@ +# Review PR #243 — Editor, NMT location: assign the place from the camera; //$g found in included files + +Autor: @miloszunio · base: `feat/switch-blades` · 3 pliki, +55/−1 +https://github.com/MaSzyna-EU07/maszyna/pull/243 + +**Werdykt: Request changes (konflikty + struktura kodu)** + +## Podsumowanie +1. „Przypisz z kamery” — odwrotność „The scenery is here”: kliknięty punkt mapy = obecna pozycja kamery, z tego liczony jest początek układu (`E0 = E + x`, `N0 = N − z`). +2. „Środek zapisany w pliku scenerii” — przywraca początek z linii `//$g`. +3. `read_georeference()` szuka `//$g` także w plikach `include` (jeden poziom). + +Bardzo przydatne — dziś jeden błędny klik psuł georeferencję. + +## Co sprawdziłem +- Wzór kamery jest zgodny z istniejącą konwersją w `editormode.cpp` (`config.north + Camera.Pos.z`, `config.east - Camera.Pos.x`). ✔ +- Kolejność `{m_georeference_origin.y, m_georeference_origin.x}` zgadza się z `nmt_place_scenery` (`Point.x = east`). ✔ +- `read_georeference()` jest wołane co klatkę w `render_nmt_place`, ale ma cache `m_georeference_read`, więc pliki czytane są raz. ✔ + +## Uwagi +1. **Konflikty.** GitHub pokazuje stan `dirty` — PR wymaga rebase na aktualny `feat/switch-blades`. Jest też `split/3-switch-blades` — warto ustalić, na którą gałąź ma finalnie trafić. +2. **Zagnieżdżenie i wcięcia w `read_georeference()`.** Wewnętrzna pętla nie została wcięta po dodaniu zewnętrznego `for`, a głębokość to teraz for→for→if→if→if — Sonar zgłosi S134 i prawdopodobnie S3776. Proponuję: + - helper `std::optional include_target(std::string_view line)` w anonimowym namespace, + - helper `bool parse_georeference_line(std::string const &line, …)`, + - pętlę po plikach z wczesnym `continue`. +3. **Ścieżki include.** `Global.asCurrentSceneryPath / name` — sprawdzić wielkość liter (na Linuksie `irt/IRT_sceneria.inc` vs `irt/irt_sceneria.inc`); funkcja dla głównego pliku już ma fallback na porównanie bez wielkości liter, include'y nie. +4. **Limit `files.size() < 8`** — magiczna liczba, warto nazwaną stałą z komentarzem. +5. **Ochrona przed pomyłką u źródła.** Skoro problemem jest przypadkowe „The scenery is here”, rozważyć potwierdzenie (popup) tego przycisku, gdy sceneria ma już `//$g` albo ułożone tory. Nowe przyciski to dobra droga powrotu, ale prewencja byłaby jeszcze lepsza. +6. Opis PR zawiera link do sesji Claude Code — do usunięcia przed merge (nie wnosi nic dla recenzenta). diff --git a/pr-reviews/PR-259.md b/pr-reviews/PR-259.md new file mode 100644 index 000000000..fd77baeb8 --- /dev/null +++ b/pr-reviews/PR-259.md @@ -0,0 +1,21 @@ +# Review PR #259 — fix(cab): a diesel with only the starter button stops its engine with the main switch off command + +Autor: @jerrrrycho · base: `master` · 4 pliki, +136/−1 · Fixes eu07/maszyna#427 +https://github.com/MaSzyna-EU07/maszyna/pull/259 + +**Werdykt: Approve z drobnymi uwagami** + +## Podsumowanie +SM03/SM04/2Ls150 (i DL2, ST44, TEM2) mają w kabinie tylko `main_on_bt`. `OnCommand_linebreakeropen()` dla takich pojazdów tylko animowało przycisk i wychodziło — silnika nie dało się zgasić. Teraz dla `DieselEngine`/`DieselElectric` komenda otwiera obwód (`MainSwitch(false)`), a przycisk rozruchu nie jest animowany. + +## Co jest dobrze +- Bardzo dobra analiza przyczyny (automatyczna pompa paliwa + przetwornica podtrzymująca 110 V). +- Tabela „przed/po” z gry, lista dotkniętych pojazdów — zgodnie z `CLAUDE.md` (zmiana zachowania z istniejącymi danymi jest opisana). +- Świadome nieruszanie kwestii „pozycja 0 = stop” (to zmiana danych, wymaga decyzji). + +## Uwagi +1. **Testy nie pokrywają właściwej zmiany.** Testy sprawdzają `MainSwitch(false)` w fizyce, co działało już przed PR. Zmieniona jest gałąź w `TTrain::OnCommand_linebreakeropen()`, której testy nie dotykają (TTrain nie jest linkowany w unit testach?). Test `REQUIRE(shunter->MainCircuitOpensWithStarterOnly())` sprawdza tylko typ silnika. Proponuję albo opisać to uczciwie w komentarzu testu, albo wydzielić decyzję „czy komenda otwiera obwód” do funkcji, którą da się przetestować bez `TTrain` (np. wolna funkcja przyjmująca: jest `main_off_bt`/`main_bt`/`main_on_bt` + typ silnika). +2. **Nazwa `MainCircuitOpensWithStarterOnly()` myli.** Funkcja nie sprawdza nic o kabinie/starterze — tylko typ silnika. Czytając `Train.cpp` można pomyśleć, że sprawdza konfigurację przycisków. Propozycja: `MainSwitchOffStopsEngine()` albo `IsDieselEngine()`; warunek o samym `main_on_bt` zostaje w `Train.cpp`. +3. **ST44 / TEM2** — dotąd gaszone pompą paliwa, teraz także `m`. Warto upewnić się u twórców tych pojazdów (lub w dokumentacji), że to pożądane — w realu TEM2 ma przycisk „stop silnika”, więc raczej tak, ale niech to będzie świadome. +4. **Brak wizualnej reakcji** po `m` na tych pojazdach — uzasadnione (trzymany przycisk uruchomiłby silnik), ale gracz może sądzić, że klawisz nie działa. Może dźwięk przełącznika (`dsbSwitch`) mimo braku animacji? Teraz `fMainRelayTimer = 0` daje dźwięk przekaźnika — proszę potwierdzić, że coś słychać. +5. Test JSON `diesel_shunter.json` — dobry, ale `_comment` zawiera opis SM03; warto dodać nazwę pliku `.fiz`, z którego pochodzą parametry, żeby dało się je zaktualizować. diff --git a/pr-reviews/PR-260.md b/pr-reviews/PR-260.md new file mode 100644 index 000000000..f053d656b --- /dev/null +++ b/pr-reviews/PR-260.md @@ -0,0 +1,20 @@ +# Review PR #260 — fix(cab): on the SU45 and SU46 the open line contactors lamp lights with the contactors opened by the pressure switch + +Autor: @jerrrrycho · base: `master` · 3 pliki, +124/−1 · Fixes eu07/maszyna#263 +https://github.com/MaSzyna-EU07/maszyna/pull/260 + +**Werdykt: Approve** + +## Podsumowanie +`LineContactorsLampLit()` dla `DieselElectric` zwraca samo `!StLinFlag` — lampka styczników liniowych na SU45/SU46/SP45 świeci także, gdy styczniki otworzył presostat (PG < 3.6 bar albo hamulec > 2 bar). Lokomotywy elektryczne bez zmian. + +## Co jest dobrze +- Precyzyjna tabela stanów (przed/po) i weryfikacja w grze krok po kroku. +- Lista pojazdów w danych, których to dotyczy — zgodnie z `CLAUDE.md`. +- Testy z nowym `diesel_electric.json`, a test na niskie ciśnienie failuje bez poprawki. + +## Uwagi +1. **Rozróżnienie po `EngineType`.** Semantyka lampki to cecha obwodu konkretnej lokomotywy, a nie typu napędu. Dziś w danych to się pokrywa (tylko SU45/SU46/SP45), ale przyszła spalinowo-elektryczna lokomotywa z lampką „start możliwy” dostanie złe zachowanie. Rozważyć opcjonalny klucz w `.fiz`/`.mmd` (domyślnie: obecna heurystyka po typie silnika), albo przynajmniej komentarz „heurystyka, zmienić gdy pojawi się pojazd …”. +2. **`i-contactorsb`** — opis mówi, że dotyczy też tej lampki; w testach jest tylko `LineContactorsLampLit()`. Jeśli `i-contactorsb` używa tej samej funkcji — OK, warto to zaznaczyć w opisie testu. +3. Drobne: komentarz nad funkcją w `MOVER.h` jest długi (3 linie z historią issue). Szczegóły historyczne lepiej w commit message, w kodzie wystarczy reguła. +4. Do rozważenia: SP45 (`DumbDE`) nie był sprawdzany w grze — tylko SU46. Szybki test na SP45 przed merge. diff --git a/pr-reviews/PR-261.md b/pr-reviews/PR-261.md new file mode 100644 index 000000000..f5ba5911d --- /dev/null +++ b/pr-reviews/PR-261.md @@ -0,0 +1,24 @@ +# Review PR #261 — feat(cab): 401Da oil pump lamp and slotted speedometer lever + +Autor: @jerrrrycho · base: `master` · 7 plików, +218/−1 · Closes eu07/maszyna#546 +https://github.com/MaSzyna-EU07/maszyna/pull/261 + +**Werdykt: Request changes — wydzielić poprawkę AVX2, reszta OK** + +## Podsumowanie +- Nowa lampka `i-oilpump:` (pompa smarowania wstępnego; gaśnie po uruchomieniu silnika). +- Nowy klucz `.mmd` `animslottedlever:` — dźwignia z jarzmem napędzana czopem wiązara (napęd prędkościomierza). +- Dodatkowe testy opóźnienia startu sprężarki (`CompressorStartDelay`, już na master z #217). +- **Poprawka builda:** `GLM_FORCE_AVX2` tylko gdy kompilator faktycznie celuje w AVX2. + +## Uwagi +1. **Poprawka AVX2 powinna być osobnym PR-em.** To ważna poprawka (crash na CPU bez AVX2 przy `ENABLE_AVX2=OFF`), niezwiązana z 401Da. Osobny PR szybciej przejdzie review, łatwiej ją zcherry-pickować na gałąź wydania 26.11 i na `feat/gl46-migration`, i będzie widoczna w changelogu. Sama zmiana (`#elif defined(__AVX2__)`) jest poprawna dla MSVC (`/arch:AVX2`) i GCC (`-mavx2`). +2. **`animslottedlever:` — kompatybilność OK** (nowy opcjonalny klucz; bez niego nic się nie zmienia). Proszę o: + - wpis do dokumentacji formatu `.mmd` (wiki), skoro żaden pojazd jeszcze go nie używa, + - informację, czy brak submodelu ma logować błąd `Bad model` — dla nowej funkcji to OK (prawdziwy błąd danych). +3. **Matematyka bez testu.** `slottedlever_angle()` to czysta funkcja geometrii, a nie jest testowana tylko dlatego, że siedzi w `TDynamicObject`. Proponuję przenieść ją do małego nagłówka (np. `utilities/` albo wolna funkcja w anonimowym namespace + deklaracja w nagłówku testowalnym) i dopisać testy: oś wewnątrz okręgu → pełny obrót, okrąg przez oś → wahanie ±90°. +4. **`float3`** w `SetRotate(float3(1,0,0), angle)` — #224 właśnie migruje `float3d` na `glm`. Nowy kod lepiej od razu na `glm::vec3`, żeby nie powiększać konfliktu. +5. **`update_slottedlevers()`** jest wołane w `ABuLittleUpdate()` poza sprawdzeniem odległości (`ObjSqrDist < 50*50`) — dla pustego wektora to nic nie kosztuje, ale dla pojazdów z dźwigniami liczy się zawsze. Rozważyć wołanie razem z animacjami kół (te same warunki LOD). +6. **Indeks `wheelid`**: warunek `lever.axle < iAnimType[ANIM_WHEELS] && lever.axle < pAnimations.size()` zakłada, że animacje kół są na początku `pAnimations` — proszę o komentarz albo assert. +7. **Lampka `i-oilpump:`** pokazuje `OilPump.is_active`. Dla pojazdów, gdzie pompa oleju pracuje cały czas (nie tylko smarowanie wstępne), lampka będzie świecić stale — to raczej poprawne, ale warto opisać semantykę w dokumentacji `.mmd`. +8. Opis: „SM42 stood in for the diesel” — test z prawdziwym modelem 401Da jest w „Known issues”; OK, ale niech ktoś od modelu 401Da potwierdzi przed wydaniem. diff --git a/pr-reviews/PR-262.md b/pr-reviews/PR-262.md new file mode 100644 index 000000000..789dd6a05 --- /dev/null +++ b/pr-reviews/PR-262.md @@ -0,0 +1,18 @@ +# Review PR #262 — AI: use the locomotive releaser only when starting off + +Autor: @pwarszawski · base: `master` · 1 plik, +4/−1 +https://github.com/MaSzyna-EU07/maszyna/pull/262 + +**Werdykt: Approve** + +## Podsumowanie +Reguła „wyluzuj lokomotywę, to szybciej ruszymy” w `TController::control_releaser()` nie miała warunku prędkości i odpalała odluźniacz w biegu (104 z 122 włączeń powyżej 1 km/h w pomiarze), opróżniając zbiornik sterujący lokomotywy przy zahamowanym pociągu. Dodano `mvOccupied->Vel < 1.0`. + +## Co jest dobrze +- Minimalna, celowana zmiana zgodna z intencją oryginalnego komentarza. +- Bardzo dobry opis z pomiarem master vs master+PR. + +## Uwagi +1. **Próg 1 km/h a ruszanie.** Przy ruszaniu AI najpierw zadaje jazdę, a lokomotywa może już pełznąć (>1 km/h), zanim cylinder spadnie poniżej 0.4 bar — wtedy reguła przestanie działać w pół ruszania. Proszę potwierdzić w logu z pomiaru, że czas ruszania (od zadania jazdy do np. 10 km/h) się nie wydłużył, szczególnie dla ciężkich składów towarowych z ESt3. Ewentualnie próg np. 5 km/h byłby bezpieczniejszy, a dalej odcinałby wszystkie przypadki z pomiaru (7–100 km/h). +2. **Kolejność merge z #263/#264/#265.** Te PR-y tego samego autora też dotykają `control_releaser()` / logiki hamowania AI. Warto zaznaczyć w opisach, w jakiej kolejności mają wchodzić i czy #262 jest zawarty w pozostałych (żeby uniknąć konfliktów i podwójnego review). +3. Drobne: komentarz po angielsku obok polskiego — OK, ale można go skrócić do jednej linii. diff --git a/pr-reviews/PR-263.md b/pr-reviews/PR-263.md new file mode 100644 index 000000000..2d9829eb2 --- /dev/null +++ b/pr-reviews/PR-263.md @@ -0,0 +1,35 @@ +# Review PR #263 — ESt3 distributor: spring preload, minimum stage, releaser and AL2 timing; AI predicts the new characteristic + +Autor: @pwarszawski · base: `master` (zbudowany na #187) · przyrost ponad #187: 4 pliki, ~+60/−15 +https://github.com/MaSzyna-EU07/maszyna/pull/263 + +**Werdykt: Approve merytorycznie; przed merge — testy golden / CI i decyzja o kolejności z #264** + +> Recenzja dotyczy tylko przyrostu ponad #187 (interdiff policzony lokalnie). + +## Podsumowanie +Model rozrządu `TNESt3`: +- wstępny nacisk sprężyn 0.21 bar zamiast 0.05 (`dPSprezyny`), `BVM` liczone z tego samego offsetu → pełne hamowanie dalej daje `MaxBP`, +- minimalny stopień 0.28 bar (`pStopnia`), pełne odluźnienie gdy cel = 0, +- odluźniacz automatyczny: ZS 15 l z 5.0 do 0.4 bar w ~23 s (było 5.6 s), +- `dTN` 1.65 → 1.62 (literówka), +- AL2: komora impulsowa 1 l, dysze cylindra przeskalowane wg tablicy 3-9. + +AI: `TNESt3::GetBrakeRatio()` zwraca rzeczywisty udział ciśnienia w cylindrze dla danego spadku w PG, więc tablice hamowania AI odpowiadają charakterystyce zaworu. + +## Co sprawdziłem +- `GetBrakeRatio(1.0) == 1.0`: `spadek = MaxBP/BVM + 0.21 = HPP − LPP`, `cel = (spadek − 0.21)·BVM = MaxBP`. ✔ Dzięki temu wywołania `BrakeForceR(1.0, …)` w `DynObj.cpp` i `Train.cpp` (wskaźniki siły hamowania) się nie zmieniają. +- Nowe progi w `CheckState()` to stare progi przeskalowane do domeny ciśnienia w cylindrze (`0.01 → 0.01·BVM`) — równoważne poza nowym minimalnym stopniem. ✔ +- Zmiana fizyki dotyczy każdego pojazdu z ESt3 bez zmian w `.fiz` — opis PR mówi to wprost, zgodnie z `CLAUDE.md`. ✔ + +## Uwagi +1. **Testy golden / CI.** Na `master` są ślady golden (`tests/unit/data/golden/consist/`, `test_consist.cpp` loguje m.in. `BrakeForceR0.25`), a także `test_brakes.cpp` / `test_brake_cycle.cpp` z pojazdami ESt3 (`401W`, `430W`…). PR nie zmienia żadnego testu, a GitHub pokazuje stan `unstable`. Proszę po rebase na master sprawdzić, które testy się zmieniły, i zaktualizować golden świadomie (z opisem w PR, co się zmieniło i dlaczego). +2. **Testy akceptacyjne jako unit testy.** W opisie jest „valve test bench that runs the acceptance procedures” (77 → 40 wyników poza tolerancją). Bardzo proszę dołożyć choć część tych prób (stopień minimalny — próby 12/22, odluźniacz — próba 27, czasy AL2 — tablica 3-9) do `tests/unit`. To najlepsza ochrona tej poprawki przed regresją. +3. **Kolejność merge.** Opis przyznaje, że sam #263 pogarsza precyzję zatrzymań (403 → 363 w oknie 8–15 m) i daje problem z ciężkim G na śliskim spadku, który naprawia dopiero #264. Proponuję merge #263 i #264 razem (albo w jednym oknie wydania), a nie #263 osobno przed 26.11. +4. **Wolniejszy odluźniacz a reszta AI.** 4× wolniejsze opróżnianie ZS wpływa też na reguły `control_releaser()` przy łączeniu/rozłączaniu i na #265 (ruszanie pod górę z odluźniaczem). Czy scenariusze manewrowe (sprzęganie/odczepianie, odhamowanie składu po połączeniu) zostały sprawdzone? Gracz używający odluźniacza na wagonach też odczuje różnicę — warto to wspomnieć w changelogu. +5. **Kod:** + - `static double const dPSprezyny/pStopnia` → `constexpr double` (wytyczne GCC/Sonar w `CLAUDE.md`) i najlepiej w anonimowym namespace. + - Współczynniki AL2 `0.914 / 0.885 / 0.82 / 0.8` — dopisać w komentarzu, z jakich czasów tablicy 3-9 wynikają (albo jak je wyliczono), żeby dało się je odtworzyć. + - `0.0049` w odluźniaczu — podobnie, nazwana stała z odniesieniem do próby 27. + - W `BrakeForceR()` `GetBrakeRatio` jest stosowane przed korektą ED (`dbrake_automatic`), rapidem i `DMU -0.3`. Wydaje się poprawne, ale proszę potwierdzić, że kolejność jest zamierzona. +6. **Konsultacja z youBy** — warto, żeby youBy zostawił w PR formalne approve części fizycznej; to jego model. diff --git a/pr-reviews/PR-264.md b/pr-reviews/PR-264.md new file mode 100644 index 000000000..446c3e910 --- /dev/null +++ b/pr-reviews/PR-264.md @@ -0,0 +1,27 @@ +# Review PR #264 — AI braking: smaller steps, earlier, while approaching a stop or a lower speed + +Autor: @pwarszawski · base: `master` (zbudowany na #187 + #263 + #262) · przyrost: `Driver.cpp/.h`, ~+70/−10 +https://github.com/MaSzyna-EU07/maszyna/pull/264 + +**Werdykt: Approve z uwagami (struktura logiki, stałe)** + +> Recenzja dotyczy tylko przyrostu ponad #263 (interdiff policzony lokalnie). Przyrost zawiera też zmianę z #262 (odluźniacz < 1 km/h) — po merge #262 zniknie z diffu. + +## Podsumowanie +- Margines dociążenia hamulca przy dojeździe do niższej prędkości: pół stopnia zamiast pełnego (`brake_increase_margin()`). +- Śledzenie „niedoboru” opóźnienia co 3 s (`fShortfall`, `iShortfallGrowth`) — rosnący niedobór przy nieruszanym kranie → mały stopień od razu. +- `DecBrake()`: pozycja poniżej pierwszej traktowana jak pierwsza (minimalny stopień z #263). +- Utrzymywanie prędkości na spadku: wzmocnienie ograniczone do `1/(3.6·horizon)`, czyli różnica zdejmowana w czasie reakcji hamulca. + +## Co jest dobrze +- Każda zmiana ma uzasadnienie fizyczne w komentarzu. +- Wynik na śliskim spadku wyraźny (defekty 7.0 % → 1.0 %, >30 m przed semaforem 7.3 % → 4.3 %). + +## Uwagi +1. **Wyniki mieszane na suchym spadku.** „Stops more than 30 m short” 3.2 % → 5.4 % i defekty 3.9 % → 4.2 %. Przy 3 vs 4 przebiegach to może być szum — proszę podać rozrzut między przebiegami albo dorobić 4. przebieg, żeby porównanie było symetryczne. +2. **Dwie ścieżki „dodaj stopień, bo pociąg nie nadąża”.** Jedna w `IncBrake()` (`fShortfall > 0.02 && fBrakeHeldTime > horizon`), druga w `control_braking_force()` (`iShortfallGrowth >= 2 && …`), która i tak woła `IncBrake()` przez `cue_action`. Trudno to śledzić — proszę o jeden helper typu `bool is_falling_behind() const` używany w obu miejscach, albo komentarz opisujący, jak się te warunki składają. +3. **Deklaracja zapowiadająca `static brake_response_horizon(...)`** w środku pliku — lepiej przenieść definicję wyżej (lub wszystkie helpery do anonimowego namespace na początku pliku). +4. **Magiczne liczby**: `0.02`, `0.005`, `3.0` s, `2` sprawdzenia, `0.04` — `constexpr` z nazwami (część jest współdzielona z #187/#265). +5. **Zmiana w warunku `room`** (usunięcie `VelNext > 0.0`) zmienia też dojazd do *niższej prędkości* z już zaciągniętym hamulcem — opis PR o tym nie wspomina. Proszę dopisać jedno zdanie. +6. **Zakres `brake_increase_margin()`** — tylko `TBrakeSystem::Pneumatic`; dla EP/ED zostaje pełny stopień. OK, ale warto to zaznaczyć w komentarzu (EP reaguje od razu, więc nie potrzebuje). +7. Komentarze nowych pól w `Driver.h` są po angielsku, sąsiednie po polsku — drobne, jednolitość. diff --git a/pr-reviews/PR-265.md b/pr-reviews/PR-265.md new file mode 100644 index 000000000..b5df4b81c --- /dev/null +++ b/pr-reviews/PR-265.md @@ -0,0 +1,32 @@ +# Review PR #265 — AI on up gradients: hold a train coming to a stand, start on the train brake + +Autor: @pwarszawski · base: `master` (zbudowany na #264 → #263 → #187, #262) · przyrost: `Driver.cpp/.h`, ~+150/−5 +https://github.com/MaSzyna-EU07/maszyna/pull/265 + +**Werdykt: Request changes — ryzyko dla sprzęgów i stan `bUphillStart`** + +> Recenzja dotyczy tylko przyrostu ponad #264 (interdiff policzony lokalnie). + +## Podsumowanie +1. **Zatrzymanie pod górę:** hamulec zespolony zaciągany zanim samo wzniesienie zatrzyma pociąg; bez luzowania poniżej 10 km/h (`uphill_stop_hold()`); bez „dociągania” gdy hamulec już działa (`uphill_brake_committed()`); hamulec lokomotywy < 3 km/h i natychmiast przy staczaniu. +2. **Ruszanie pod górę** (`control_uphill_start()`): hamulec zespolony zostaje, lokomotywa odhamowana (dodatkowy + odluźniacz), moc podawana przeciw zahamowanemu składowi; zwolnienie hamulca gdy Ft > 1.1 × siła z pochylenia, pociąg ruszy, albo po 45 s. + +Wyniki bardzo dobre: staczanie po zatrzymaniu 51 → 0, ruszanie ze staczaniem 7/62 → 0/155. + +## Uwagi blokujące +1. **Pełna moc przeciw zahamowanemu składowi przez 10–18 s.** Opis sam to przyznaje („still applies full power at once”). Dla ciężkich towarów to klasyczny przepis na zerwanie sprzęgu (por. dyskusję w #187 i fix #255). Proszę o: + - statystykę maks. sił w sprzęgach podczas ruszania (base vs PR) dla najcięższych składów (ET42 + 30 wagonów, ET41 + 25), + - albo ograniczenie mocy do wartości potrzebnej na utrzymanie (np. do ~1.1–1.2 × `holdingforce`) dopóki `bUphillStart`. +2. **`bUphillStart` może utknąć na `true`.** Flaga jest resetowana tylko w `control_uphill_start()`, wołanym z `control_tractive_force()`. Jeśli AI zostanie wyłączone w trakcie ruszania (Shift+Q / Q, a po #231 także z menu), flaga zostaje, a w `determine_consist_state()` `heldforstart` dalej: + - ignoruje niezluzowane hamulce przy ustalaniu `Ready`, + - zeruje `fReady`. + Proszę resetować `bUphillStart` przy przejęciu sterowania przez gracza / zmianie rozkazu / `OrderCurrentGet()`, a w `determine_consist_state()` warunkować to dodatkowo `AIControllFlag`. +3. **`Ready` ignoruje *każde* hamowanie** podczas ruszania (także `vehicle->Fb > 10 kN`, np. zaciągnięty hamulec ręczny wagonu albo niezluzowany wagon z innego powodu). Przez 45 s AI będzie ciągnąć przeciw nim. Lepiej ignorować tylko hamulec zespolony (`b_on|b_hld` przy zaciągniętym kranie), a `Fb` od hamulca ręcznego/postojowego nadal blokować. + +## Pozostałe uwagi +4. **Niespójne progi pochylenia:** `-0.005` (`uphill_stop_hold`, lokomotywa przy staczaniu), `-0.01` (`control_uphill_start`), `-0.025` (`uphill_brake_committed`, #187), `-0.05` (staczanie w `control_braking_force`). Proszę o nazwane stałe i krótkie uzasadnienie różnic. +5. **Odluźniacz przy ruszaniu a #263.** Po #263 odluźniacz ESt3 opróżnia ZS ~23 s zamiast ~6 s — czy pomiary „10–18 s pulling against the brake” były robione już z nowym odluźniaczem (stos sugeruje, że tak — proszę potwierdzić)? +6. **Złożoność (Sonar S3776/S134/S1067):** warunek `independentbrakeapply` w `control_braking_force()` ma 9 członów z mieszanym `&&`/`||` — wydzielić do `bool should_hold_on_independent_brake(double velocity) const`. +7. **`break` w `DecBrake()`** — działa, bo jesteśmy w `switch`, ale przy dwóch nowych `break` przed właściwą logiką czytelniej byłoby jedno wczesne wyjście z komentarzem. +8. `fUphillStartTime` mierzony `ElapsedTime` — przy pauzie/przyspieszeniu czasu OK, ale 45 s powinno być stałą nazwaną (`uphill_start_timeout`). +9. **Kolejność merge:** całego stosu #187 → #262 → #263 → #264 → #265 — zaproponuję maintainerom zmergować to w jednym oknie po kolei, z rebase każdego PR po poprzednim, żeby CI/Sonar liczyły tylko przyrost. diff --git a/pr-reviews/README.md b/pr-reviews/README.md new file mode 100644 index 000000000..10dd03576 --- /dev/null +++ b/pr-reviews/README.md @@ -0,0 +1,46 @@ +# Recenzje otwartych PR-ów — MaSzyna-EU07/maszyna (stan na 7.11.2026) + +23 PR-y (bez draftów #216, #221, #203, #173, #137). Każda recenzja w osobnym pliku `PR-.md`, gotowa do wklejenia na GitHuba. +Punktem odniesienia były zasady zespołu z `CLAUDE.md` (GCC, Sonar, kompatybilność danych, renderer GL4.6). + +| PR | Tytuł (skrót) | Autor | Werdykt | +|---|---|---|---| +| #265 | AI pod górę: trzymanie i ruszanie na hamulcu | pwarszawski | Request changes | +| #264 | AI: mniejsze, wcześniejsze stopnie hamowania | pwarszawski | Approve z uwagami | +| #263 | ESt3: nacisk sprężyn, stopień min., odluźniacz, AL2 | pwarszawski | Approve; golden/CI do sprawdzenia | +| #262 | AI: odluźniacz tylko przy ruszaniu | pwarszawski | Approve | +| #261 | 401Da: lampka pompy oleju, dźwignia prędkościomierza | jerrrrycho | Request changes (wydzielić AVX2) | +| #260 | SU45/SU46: lampka styczników liniowych | jerrrrycho | Approve | +| #259 | SM03: gaszenie silnika wyłącznikiem | jerrrrycho | Approve z drobnymi | +| #243 | Edytor NMT: przypisz z kamery, `//$g` w include | miloszunio | Request changes | +| #241 | Edytor: lista wypełnionych obszarów | miloszunio | Request changes | +| #240 | GL46: głębia kursora edytora | miloszunio | Approve po teście | +| #239 | Refaktor długich funkcji Mover | jerrrrycho | Request changes (rebase) | +| #234 | Przełączniki świateł 3-pozycyjne | dos1 | Approve z uwagami | +| #231 | Przełączanie AI z menu | dos1 | Request changes (drobne) | +| #230 | Rozmiar Słońca i Księżyca | dos1 | Approve (+ odpowiednik GL46) | +| #228 | XYZ→RGB w sRGB | dos1 | Approve z drobnymi | +| #227 | Wartości turbidity | dos1 | Approve po opisie | +| #224 | float3d → glm | docentYT | Request changes (build VR) | +| #187 | AI: spadki, dojazdy, 8–15 m przed semaforem | pwarszawski | Approve kierunku; Sonar/GCC | +| #172 | UART Hardware Protocol v2 | Hirek193 | Request changes | +| #167 | whois +48 | pwarszawski | Approve z drobnymi | +| #165 | HUD | DJ-YASHITAKA | Request changes → draft | +| #159 | Spawn składów z Lua | jakubg1 | Request changes | +| #150 | Podział OnCommands z Train | jerrrrycho | Approve kierunku; rebase | + +## Najważniejsze znaleziska +- **#165** — przypadkowe zmiany w `Globals.h`: `targetfps` 60 → 0 i usunięty klucz UART `dynamicbrake` (psuje odczyt `eu07.ini`). +- **#224** — usuwa `ReplaceMatrix()`, którego używa `vr/openvr_imp.cpp` → build z OpenVR się nie skompiluje (CI tego nie łapie). +- **#265** — flaga `bUphillStart` może zostać `true` po przejęciu sterowania przez gracza; pełna moc przeciw zahamowanemu składowi przez 10–18 s to ryzyko dla sprzęgów. +- **#150** — po 10.10 doszedł `world/RoadPoint.cpp` ze starym `#include "vehicle/Train.h"` → po merge build się wysypie bez rebase. +- **#263** — zmienia fizykę każdego ESt3, a nie rusza testów golden; GitHub pokazuje `unstable`. +- **#159** — `insert_trainset_full()` nie zatrzymuje się na `endtrainset`; `lua_tostring` może zwrócić `NULL`. + +## Proponowana kolejność merge (konflikty) +1. Małe, niezależne: #262, #260, #259, #167, #228, #227, #230, #234, #231 (po poprawce). +2. Stos AI/hamulce, po kolei z rebase: #187 → #263 → #264 → #265 (najlepiej w jednym oknie). +3. Poprawka AVX2 z #261 jako osobny PR, potem #261. +4. Refaktory na końcu, z ponownym puszczeniem skryptu / rebase: #150 (Train.cpp), #239 (Mover.cpp), #224 (float3 → glm). +5. Gałęzie poboczne: #240, #241 → `feat/gl46-migration`; #243 → `feat/switch-blades`. +6. Do rozmowy z autorami / draft: #165, #172, #159. diff --git a/rendering/common/windscreenwater.cpp b/rendering/common/windscreenwater.cpp index 50c1147e8..9a52eb3fe 100644 --- a/rendering/common/windscreenwater.cpp +++ b/rendering/common/windscreenwater.cpp @@ -329,8 +329,9 @@ void windscreen_water::update(double const Seconds) build(*cab); } } - if (m_glasscount == 0) + if ((vehicle == nullptr) || (m_glasscount == 0)) { + // no glasses without a cab, which comes with the vehicle return; } diff --git a/rendering/opengl46/opengl46renderer.cpp b/rendering/opengl46/opengl46renderer.cpp index 180a32144..7ad258abd 100644 --- a/rendering/opengl46/opengl46renderer.cpp +++ b/rendering/opengl46/opengl46renderer.cpp @@ -3705,7 +3705,8 @@ void opengl46_renderer::Render_Instances_Indirect() part( m_cullparts.record ); using command_set = opengl46_instance_cull::command_set; - auto const twophase { m_instancecull.two_phase() }; + // two phases only with the occlusion test, which needs the viewport + auto const twophase { useocclusion && m_instancecull.two_phase() }; auto const draws { false == m_indirectdraws.empty() }; // the depth comes first in the depth pass of the whole scene, which has its state set and leaves the colour to the colour // pass (m_indirectcolourpending), and with the depth prepass of the instances: the colour pass shades only what stays in @@ -6380,7 +6381,8 @@ void opengl46_renderer::Render_local_shadows() // only the light buffers with the lamps read the maps auto const *vehicle{Global.gfx_lights_clustered ? m_locallights.cab_lamp_vehicle() : nullptr}; m_localshadows.update_cab(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), m_locallights.cab_lights(), m_framestamp); - if (m_localshadows.cab_draws().empty()) + // without a vehicle there are no draws either + if ((vehicle == nullptr) || m_localshadows.cab_draws().empty()) return; glDebug("render lamp shadows"); @@ -6423,7 +6425,8 @@ void opengl46_renderer::Render_cab_probe() // the lights of the cab light the probe when they're lights of the frame std::vector const nolights; auto const &lights{(vehicle != nullptr) && (m_locallights.cab_lamp_vehicle() == vehicle) ? m_locallights.cab_lights() : nolights}; - if (false == m_cabprobe.update_wanted(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), center, extent, lights, m_framestamp)) + // update_wanted() wants no update without a vehicle, but is called anyway to forget the probe + if ((false == m_cabprobe.update_wanted(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), center, extent, lights, m_framestamp)) || (vehicle == nullptr)) return; glDebug("render cab probe"); diff --git a/rendering/vulkan14/vulkan14cabprobe.cpp b/rendering/vulkan14/vulkan14cabprobe.cpp index 891f6a36a..64268dbbd 100644 --- a/rendering/vulkan14/vulkan14cabprobe.cpp +++ b/rendering/vulkan14/vulkan14cabprobe.cpp @@ -108,7 +108,14 @@ namespace vk14 cab_probe_maps::~cab_probe_maps() { - destroy(); + try + { + destroy(); + } + catch (std::exception const &) + { + // out of memory while letting the maps go: what's left is released with the device, which beats terminating + } } void cab_probe_maps::init(VkDevice const Device, allocator &Memory, deletion_queue &Deletions) @@ -351,7 +358,8 @@ void vulkan14_renderer::render_cab_probe() // the lights of the cab light the probe when they're lights of the frame std::vector const nolights; auto const &lights{(vehicle != nullptr) && (m_locallights.cab_lamp_vehicle() == vehicle) ? m_locallights.cab_lights() : nolights}; - if (false == probe.update_wanted(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), center, extent, lights, m_framestamp)) + // update_wanted() wants no update without a vehicle, but is called anyway to forget the probe + if ((false == probe.update_wanted(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), center, extent, lights, m_framestamp)) || (vehicle == nullptr)) { return; } diff --git a/rendering/vulkan14/vulkan14commands.cpp b/rendering/vulkan14/vulkan14commands.cpp index b4800667d..11fb04b9b 100644 --- a/rendering/vulkan14/vulkan14commands.cpp +++ b/rendering/vulkan14/vulkan14commands.cpp @@ -255,6 +255,11 @@ void compute_queue_scope(VkPipelineStageFlags2 &Stage, VkAccessFlags2 &Access) template void compute_queue_barriers(Barrier_ *Barriers, std::uint32_t const Count) { + // none of the kind: the stream holds no data for them + if (Barriers == nullptr) + { + return; + } for (std::uint32_t i = 0; i < Count; ++i) { compute_queue_scope(Barriers[i].srcStageMask, Barriers[i].srcAccessMask); diff --git a/rendering/vulkan14/vulkan14grass.cpp b/rendering/vulkan14/vulkan14grass.cpp index 3c0e99cae..f8faffd38 100644 --- a/rendering/vulkan14/vulkan14grass.cpp +++ b/rendering/vulkan14/vulkan14grass.cpp @@ -11,8 +11,6 @@ http://mozilla.org/MPL/2.0/. #include "rendering/vulkan14/vulkan14grass.h" #include "utilities/Logs.h" -#include - namespace vk14 { @@ -132,7 +130,7 @@ grass_field::chunk_images const *grass_field::images(chunk_draw const &Draw) } table[4 * 4 + 0] = Draw.share; table[4 * 4 + 1] = Draw.chunk; - if ((false == chunk.table_made) || (std::memcmp(table.data(), chunk.table.data(), sizeof(table)) != 0) || (chunk.images[3] == nullptr)) + if ((false == chunk.table_made) || (table != chunk.table) || (chunk.images[3] == nullptr)) { auto map{chunk.images[3]}; if (map == nullptr) diff --git a/rendering/vulkan14/vulkan14indirect.cpp b/rendering/vulkan14/vulkan14indirect.cpp index 38028907b..f88a33369 100644 --- a/rendering/vulkan14/vulkan14indirect.cpp +++ b/rendering/vulkan14/vulkan14indirect.cpp @@ -665,7 +665,8 @@ void vulkan14_renderer::render_instances_indirect() // the draws, recorded while the gpu culls record_indirect_draws(cull, false); - auto const twophase{cull.two_phase()}; + // two phases only with the occlusion test, which needs the targets + auto const twophase{useocclusion && cull.two_phase()}; auto const draws{false == m_indirectdraws.empty()}; // the depth comes first in the depth pass of the whole scene, which leaves the colour to the colour pass, and with the depth // prepass of the instances diff --git a/rendering/vulkan14/vulkan14lamps.cpp b/rendering/vulkan14/vulkan14lamps.cpp index 92abdca73..9857d5108 100644 --- a/rendering/vulkan14/vulkan14lamps.cpp +++ b/rendering/vulkan14/vulkan14lamps.cpp @@ -111,7 +111,8 @@ void vulkan14_renderer::render_local_shadows() // only the light buffers with the lamps read the maps auto const *vehicle{Global.gfx_lights_clustered ? m_locallights.cab_lamp_vehicle() : nullptr}; lamps.update_cab(vehicle, (vehicle != nullptr ? vehicle->mdKabina : nullptr), m_locallights.cab_lights(), m_framestamp); - if (lamps.cab_draws().empty()) + // without a vehicle there are no draws either + if ((vehicle == nullptr) || lamps.cab_draws().empty()) { return; } diff --git a/rendering/vulkan14/vulkan14memory.cpp b/rendering/vulkan14/vulkan14memory.cpp index 39f5e69ad..7d6cc88d1 100644 --- a/rendering/vulkan14/vulkan14memory.cpp +++ b/rendering/vulkan14/vulkan14memory.cpp @@ -53,20 +53,29 @@ void add(std::uint64_t const Handle, entry &&Entry) g_live[Handle] = std::move(Entry); } -void remove(std::uint64_t const Handle) +// called by the destructors of buffers and images, so it lets no exception out: the registry only serves the report after +// a device loss, and a resource it fails to move to the freed ones mustn't stop the resource from being released +void remove(std::uint64_t const Handle) noexcept { - std::lock_guard lock(g_mutex); - auto const lookup{g_live.find(Handle)}; - if (lookup == g_live.end()) + try { - return; + std::scoped_lock lock(g_mutex); + auto const lookup{g_live.find(Handle)}; + if (lookup == g_live.end()) + { + return; + } + lookup->second.freed = std::chrono::steady_clock::now(); + g_freed.push_back(std::move(lookup->second)); + g_live.erase(lookup); + if (g_freed.size() > freed_kept) + { + g_freed.pop_front(); + } } - lookup->second.freed = std::chrono::steady_clock::now(); - g_freed.push_back(std::move(lookup->second)); - g_live.erase(lookup); - if (g_freed.size() > freed_kept) + catch (std::exception const &) { - g_freed.pop_front(); + // the entry stays among the live ones, which the report can live with } } diff --git a/rendering/vulkan14/vulkan14shaders.cpp b/rendering/vulkan14/vulkan14shaders.cpp index 08d9e7797..6bac1925c 100644 --- a/rendering/vulkan14/vulkan14shaders.cpp +++ b/rendering/vulkan14/vulkan14shaders.cpp @@ -340,7 +340,14 @@ std::string shader_library::info() const pipeline_library::~pipeline_library() { - shutdown(); + try + { + shutdown(); + } + catch (std::exception const &) + { + // writing the pipeline cache failed on the way out: the cache is only rebuilt next time, which beats terminating + } } bool pipeline_library::init(device const &Device, std::string &Reason) diff --git a/shaders/compile_spirv.py b/shaders/compile_spirv.py index f77c8215c..061783c1a 100644 --- a/shaders/compile_spirv.py +++ b/shaders/compile_spirv.py @@ -33,7 +33,7 @@ INCLUDE = re.compile(r'^[ \t]*#include[ \t]*[<"]([^>"]+)[>"]', re.M) def file_hash(path): # fnv-1a 64 of the text with the line ends made \n, the same as gfx::shader_file_hash() - with open(path, 'rb') as f: + with open(path, 'rb') as f: # NOSONAR: a developer tool reading the shader files of the directory it is given data = f.read().replace(b'\r\n', b'\n') h = 14695981039346656037 for b in data: @@ -49,7 +49,7 @@ def dependencies(directory, name, found=None): if name in found: return found found.append(name) - with open(os.path.join(directory, name), 'r', encoding='utf-8', errors='replace') as f: + with open(os.path.join(directory, name), 'r', encoding='utf-8', errors='replace') as f: # NOSONAR: a developer tool reading the shader files of the directory it is given text = f.read() for include in INCLUDE.findall(text): if include == 'common': @@ -79,7 +79,7 @@ def main(): args = parser.parse_args() glslc = find_glslc(args.glslc) - if not glslc: + if not glslc or not os.path.isfile(glslc): print('glslc not found: install the Vulkan SDK (VULKAN_SDK) or shaderc, or give --glslc', file=sys.stderr) return 1 @@ -92,7 +92,7 @@ def main(): binaries = os.path.join(args.shaders, binarydir) if not os.path.isdir(sources): continue - os.makedirs(binaries, exist_ok=True) + os.makedirs(binaries, exist_ok=True) # NOSONAR: a developer tool writing the binaries beside the shaders it is given manifest = ['# made by compile_spirv.py: binary, source file it was compiled from, fnv-1a 64 of the source (line ends as \\n)'] names = sorted(n for n in os.listdir(sources) if os.path.splitext(n)[1] in STAGES) for name in names: @@ -105,13 +105,13 @@ def main(): if os.path.splitext(name)[1] in ('.vert', '.frag'): text = '' for dep in deps: - with open(os.path.join(sources, dep), 'r', encoding='utf-8', errors='replace') as f: + with open(os.path.join(sources, dep), 'r', encoding='utf-8', errors='replace') as f: # NOSONAR: a developer tool reading the shader files of the directory it is given text += f.read() jobs += [(v, ['-D%s=1' % v]) for v in variants if (not onlyused) or (v in text)] for variant, defines in jobs: binary = name + ('.' + variant if variant else '') + '.spv' command = [glslc, '--target-env=' + env] + defines + [os.path.join(sources, name), '-o', os.path.join(binaries, binary)] - result = subprocess.run(command, capture_output=True, text=True) + result = subprocess.run(command, capture_output=True, text=True) # NOSONAR: runs the glslc named by --glslc or found in the Vulkan SDK, without a shell if result.returncode != 0: failed.append('%s/%s' % (binarydir, binary)) sys.stderr.write(result.stderr) @@ -121,7 +121,7 @@ def main(): count += 1 for dep in deps: manifest.append('%s %s %s' % (binary, dep, file_hash(os.path.join(sources, dep)))) - with open(os.path.join(binaries, 'manifest.txt'), 'w', newline='\n') as f: + with open(os.path.join(binaries, 'manifest.txt'), 'w', newline='\n') as f: # NOSONAR: a developer tool writing the binaries beside the shaders it is given f.write('\n'.join(manifest) + '\n') print('%d binaries compiled' % count) diff --git a/tests/unit/test_alignment.cpp b/tests/unit/test_alignment.cpp index 3b091f432..2bdf54d0d 100644 --- a/tests/unit/test_alignment.cpp +++ b/tests/unit/test_alignment.cpp @@ -62,8 +62,12 @@ std::vector traced(alignment::result const &Line, double const Spaci return glm::dvec2{station.position.x, station.position.z} + glm::normalize(across) * Shift; }; int count{0}; - for (auto chainage{std::max(0.0, from - 30.0)}; chainage <= to; chainage += Spacing, ++count) + auto chainage{std::max(0.0, from - 30.0)}; + while (chainage <= to) { points.push_back(along(chainage, Noise * (count % 3 == 0 ? 1.0 : count % 3 == 1 ? -0.6 : 0.2))); + chainage += Spacing; + ++count; + } points.push_back(along(to + 30.0, Noise * 0.5)); points.push_back(along(to + 60.0, -Noise * 0.5)); return points; @@ -750,7 +754,7 @@ TEST_CASE("alignment: a curve laid into a curved free end ends at it with its cu } TEST_CASE("alignment: the pieces of a compound curve are never of no length, whatever its arcs and transitions") { - std::mt19937 random{20261025}; + std::mt19937 random{20261025}; // NOSONAR: deterministic test input, not security-relevant auto const uniform = [&](double const Low, double const High) { return std::uniform_real_distribution(Low, High)(random); }; auto const pick = [&](int const Count) { return std::uniform_int_distribution(0, Count - 1)(random); }; int bad{0}, valid{0}; diff --git a/tests/unit/test_switch_curve.cpp b/tests/unit/test_switch_curve.cpp index 053244431..dbc662802 100644 --- a/tests/unit/test_switch_curve.cpp +++ b/tests/unit/test_switch_curve.cpp @@ -15,10 +15,12 @@ std::vector line(double const Straight, double const double heading{0.0}; auto const curvature = [&](double const S) { return S <= Straight ? 0.0 : S <= Straight + Transition ? (S - Straight) / Transition / Radius : 1.0 / Radius; }; auto const cant = [&](double const S) { return S <= Straight ? 0.0 : S <= Straight + Transition ? Cant * (S - Straight) / Transition : Cant; }; - for (double s{0.0}; s <= Straight + Transition + Arc + 1e-9; s += 0.5) { + double s{0.0}; + while (s <= Straight + Transition + Arc + 1e-9) { if (s > 0.0) heading += 0.5 * (curvature(s - 0.5) + curvature(s)) * 0.5; points.push_back({s, heading, cant(s)}); + s += 0.5; } return points; } @@ -151,8 +153,11 @@ namespace { // a track of one curvature from its start, heading as given there, a point every half metre std::vector arc(double const Length, double const Heading, double const Curvature) { std::vector points; - for (double s{0.0}; s <= Length + 1e-9; s += 0.5) + double s{0.0}; + while (s <= Length + 1e-9) { points.push_back({s, Heading + Curvature * s, 0.0}); + s += 0.5; + } return points; } diff --git a/tools/editor/usage_headers.py b/tools/editor/usage_headers.py index d996317cb..e2307f91f 100644 --- a/tools/editor/usage_headers.py +++ b/tools/editor/usage_headers.py @@ -61,7 +61,7 @@ def described(data, file): beside = os.path.join(data, 'editor', 'templates', file) if os.path.exists(beside): # a description this script wrote may be written again - with open(beside, encoding='utf-8', errors='replace') as source: + with open(beside, encoding='utf-8', errors='replace') as source: # NOSONAR: a developer tool reading the data root it is given on its command line return 'usage: {' not in source.read() try: text, _ = decode(load(template)) @@ -74,7 +74,7 @@ def read_tables(tables): uses = {} for table in tables: scenery = os.path.basename(table).replace('use_', '').replace('.tsv', '') - with open(table, encoding='utf-8') as source: + with open(table, encoding='utf-8') as source: # NOSONAR: a developer tool reading the tables it is given on its command line next(source) for row in source: cells = row.rstrip('\n').split('\t') diff --git a/utilities/utilities.cpp b/utilities/utilities.cpp index 5d0fb1824..2a74b6578 100644 --- a/utilities/utilities.cpp +++ b/utilities/utilities.cpp @@ -145,15 +145,15 @@ bool ClearFlag(int &Flag, int const Value) namespace { -thread_local std::mt19937 *t_random_engine{nullptr}; +thread_local std::mt19937 *t_random_engine{nullptr}; // NOSONAR: randomness of the simulation, not of anything security-relevant -std::mt19937 &random_engine() +std::mt19937 &random_engine() // NOSONAR: randomness of the simulation, not of anything security-relevant { return t_random_engine != nullptr ? *t_random_engine : Global.random_engine; } } // namespace -void set_thread_random_engine(std::mt19937 *Engine) +void set_thread_random_engine(std::mt19937 *Engine) // NOSONAR: randomness of the simulation, not of anything security-relevant { t_random_engine = Engine; } diff --git a/utilities/utilities.h b/utilities/utilities.h index 0615bac85..e84dfb34e 100644 --- a/utilities/utilities.h +++ b/utilities/utilities.h @@ -67,7 +67,7 @@ double Random(double a, double b); int Random(int min, int max); // the engine Random() draws from in the calling thread, in place of Global.random_engine, or none for that one again. the threads // of worker_pool have engines of their own: the one of the simulation can't be shared between threads -void set_thread_random_engine(std::mt19937 *Engine); +void set_thread_random_engine(std::mt19937 *Engine); // NOSONAR: randomness of the simulation, not of anything security-relevant /// @author lxvia /// @brief Returns a random seed from the system's entropy source, if available. Otherwise, falls back to a clock-based seed. diff --git a/utilities/workerpool.cpp b/utilities/workerpool.cpp index 74149224e..3f0d236e2 100644 --- a/utilities/workerpool.cpp +++ b/utilities/workerpool.cpp @@ -112,7 +112,7 @@ void worker_pool::work(std::function const &Work, std::size_t void worker_pool::worker(thread_data &Data) { // draws of Random() in this thread go to an engine of its own: the one of the simulation can't be shared between threads - std::mt19937 randomengine(static_cast(true_random_seed())); + std::mt19937 randomengine(static_cast(true_random_seed())); // NOSONAR: randomness of the simulation, not of anything security-relevant set_thread_random_engine(&randomengine); while (true) diff --git a/world/RailCrossings.cpp b/world/RailCrossings.cpp index e25ba09f4..d7f9d356d 100644 --- a/world/RailCrossings.cpp +++ b/world/RailCrossings.cpp @@ -673,7 +673,8 @@ knots::line check_blocks( TTrack const &Track, int const Path, int const Side, s if( false == others.empty() || false == Crossing.empty() ) { auto const ours { make_gauge_line( segment, Side * halfwidth, 0.0 ) }; for( auto const &check : Checks ) { - for( auto s { check.from }; s <= check.to; s += 0.1 ) { + auto s { check.from }; + while( s <= check.to ) { glm::dvec3 point, tangent; ours.at( s, point, tangent ); auto const inward { plan( ours.normal( s, Side ) ) }; @@ -681,6 +682,7 @@ knots::line check_blocks( TTrack const &Track, int const Path, int const Side, s || ( check.acute && blocked_by( Crossing, plan( point ), inward, Clearance ) ) ) { blocks.emplace_back( s - 0.3, s + 0.3 ); } + s += 0.1; } } } @@ -1039,12 +1041,16 @@ track_rails::switch_blade const &TTrack::blade( int const Path ) const { result.drop = ( half > 0 ? std::max( 0.0, size.crest - bladetop ) : 0.0 ); auto const lowered { reach( blade_lowered + blade_gap ) }; auto const cutto { std::min( result.heel, foot >= 0.0 ? foot + blade_spacing : result.heel ) }; - for( auto distance { result.tip }; distance < cutto + 0.5 * blade_spacing; distance += blade_spacing ) { - auto const at { std::min( distance, cutto ) }; - auto const width { level( at ) - blade_gap }; - result.extents.emplace_back( at, width ); - if( result.drop > 0.0 && ( lowered < 0.0 || at <= lowered + blade_spacing ) ) { - result.drops.emplace_back( at, result.drop * std::clamp( 1.0 - width / blade_lowered, 0.0, 1.0 ) ); + { + auto distance { result.tip }; + while( distance < cutto + 0.5 * blade_spacing ) { + auto const at { std::min( distance, cutto ) }; + auto const width { level( at ) - blade_gap }; + result.extents.emplace_back( at, width ); + if( result.drop > 0.0 && ( lowered < 0.0 || at <= lowered + blade_spacing ) ) { + result.drops.emplace_back( at, result.drop * std::clamp( 1.0 - width / blade_lowered, 0.0, 1.0 ) ); + } + distance += blade_spacing; } } auto const steps { std::max( 1, static_cast( std::ceil( ( result.heel - result.tip ) / blade_spacing ) ) ) }; diff --git a/world/RoadPoint.cpp b/world/RoadPoint.cpp index 5bf2271b2..4f3e64108 100644 --- a/world/RoadPoint.cpp +++ b/world/RoadPoint.cpp @@ -957,7 +957,7 @@ void roadpoint_node::update_crossing(double const Deltatime) auto const closed{control > 0.0 || (control == 0.0 && m_hold > 0.0)}; if (closed != m_closed) { - WriteLog("Level crossing \"" + m_name + "\" " + (closed ? "closed" : "opened") + (control != 0.0 ? " by memory cell \"" + m_memcell->name() + "\"" : " by the trains") + ", stops: " + + WriteLog("Level crossing \"" + m_name + "\" " + (closed ? "closed" : "opened") + (control != 0.0 && m_memcell != nullptr ? " by memory cell \"" + m_memcell->name() + "\"" : " by the trains") + ", stops: " + std::to_string(m_stops.size())); close(closed); } diff --git a/world/Segment.cpp b/world/Segment.cpp index 078441dab..0eb14f16d 100644 --- a/world/Segment.cpp +++ b/world/Segment.cpp @@ -807,11 +807,19 @@ bool TSegment::RenderLoftSpan( gfx::vertex_array &Output, glm::dvec3 const &Orig std::vector stations { Span.from, Span.to }; auto const step { std::max( fStep, 0.2 ) }; - for( auto distance { std::ceil( Span.from / step ) * step }; distance < Span.to; distance += step ) { - stations.push_back( distance ); + { + auto distance { std::ceil( Span.from / step ) * step }; + while( distance < Span.to ) { + stations.push_back( distance ); + distance += step; + } } - for( auto distance { std::ceil( Span.from / 0.5 ) * 0.5 }; distance < std::min( Span.to, m_dense ); distance += 0.5 ) { - stations.push_back( distance ); + { + auto distance { std::ceil( Span.from / 0.5 ) * 0.5 }; + while( distance < std::min( Span.to, m_dense ) ) { + stations.push_back( distance ); + distance += 0.5; + } } for( auto const *line : { &Span.offsets, &Span.extents, &Span.inner, &Span.drops, &Span.rises, &Span.lows } ) { for( auto const &knot : *line ) { diff --git a/world/catenary_cantilever.cpp b/world/catenary_cantilever.cpp index 2cd0177c1..9d4b08183 100644 --- a/world/catenary_cantilever.cpp +++ b/world/catenary_cantilever.cpp @@ -1404,7 +1404,8 @@ private: // f(drop) = odległość osi blachy od liny - długość kolumny; szukane miejsce zerowe w -1,0 .. 2,5 m auto f = [&]( double Drop ) { auto const p { solve( Drop, station, stay ) }; return p.ok ? length( p.pivot - messenger ) - column : 1e9; }; double previous { -1.0 }, fprevious { f( previous ) }; - for( double drop = -1.0 + 0.02; drop <= 2.5; drop += 0.02 ) { + double drop { -1.0 + 0.02 }; + while( drop <= 2.5 ) { auto const fnow { f( drop ) }; if( fprevious < 1e8 && fnow < 1e8 && ( fprevious <= 0.0 ) != ( fnow <= 0.0 ) ) { double low { previous }, high { drop }, flow { fprevious }; @@ -1420,6 +1421,7 @@ private: if( p.ok && hanging.z < 0.0 && score < bestscore ) { bestscore = score; best = p; bestdrop = low; beststation = station; besthole = static_cast( hole ); } } previous = drop; fprevious = fnow; + drop += 0.02; } } } diff --git a/world/catenary_scene.cpp b/world/catenary_scene.cpp index d965070fa..5c943a69a 100644 --- a/world/catenary_scene.cpp +++ b/world/catenary_scene.cpp @@ -2136,7 +2136,8 @@ catenary_scene_update( std::size_t const Index, catenary::suspension const &Data mark_changed( suspension.file ); if( false == before.empty() && before != Data.umbrella ) { drop_parts( suspension ); - for( auto const index : umbrella_members( before ) ) { reassemble( index ); break; } + // parasol składa jego pierwsze podwieszenie + if( auto const members { umbrella_members( before ) }; false == members.empty() ) { reassemble( members.front() ); } } reassemble( Index ); follow_chains( Index ); @@ -2153,7 +2154,8 @@ catenary_scene_remove( std::size_t const Index ) { suspension.removed = true; mark_changed( suspension.file ); if( false == umbrella.empty() ) { - for( auto const index : umbrella_members( umbrella ) ) { reassemble( index ); break; } + // parasol składa jego pierwsze podwieszenie + if( auto const members { umbrella_members( umbrella ) }; false == members.empty() ) { reassemble( members.front() ); } } // z łańcuchów wypada; łańcuch z jednym podwieszeniem przestaje istnieć if( false == suspension.data.name.empty() ) { diff --git a/world/catenary_solver.cpp b/world/catenary_solver.cpp index ae87a0059..ab927a4ef 100644 --- a/world/catenary_solver.cpp +++ b/world/catenary_solver.cpp @@ -246,7 +246,10 @@ span_result solve_span( network_type const &Type, span_input const &Input, doubl { auto nodes { messenger.nodes() }; auto const step { std::max( 0.1, Samplestep ) }; - for( double x = step; x < length; x += step ) { nodes.push_back( x ); } + { + double x { step }; + while( x < length ) { nodes.push_back( x ); x += step; } + } std::sort( nodes.begin(), nodes.end() ); nodes.erase( std::unique( nodes.begin(), nodes.end(), []( double L, double R ) { return std::abs( L - R ) < 1e-6; } ), nodes.end() ); for( auto const x : nodes ) { diff --git a/world/catenary_span.cpp b/world/catenary_span.cpp index b012c2573..d0a686c33 100644 --- a/world/catenary_span.cpp +++ b/world/catenary_span.cpp @@ -90,8 +90,11 @@ y_at( TTraction const &Traction, int const End ) { auto const neighbourend { Traction.iNext[ End ] }; if( neighbour->hvNext[ neighbourend ] != &Traction ) { return false; } if( std::abs( y_length( *neighbour ) - ylength ) > 0.01 ) { return false; } - auto const &type { definition( Traction )->type }; - auto const &neighbourtype { definition( *neighbour )->type }; + auto const *own { definition( Traction ) }; + auto const *neighbourdefinition { definition( *neighbour ) }; + if( own == nullptr || neighbourdefinition == nullptr ) { return false; } + auto const &type { own->type }; + auto const &neighbourtype { neighbourdefinition->type }; if( type.y_tension != neighbourtype.y_tension || type.y_mass != neighbourtype.y_mass || type.messenger_count != neighbourtype.messenger_count ) {