Skip to content

Fix SchLib vertex coordinate units - #55

Open
mariioo wants to merge 2 commits into
issus:masterfrom
mariioo:fix/mariusz/schlib-coordinate-units
Open

Fix SchLib vertex coordinate units#55
mariioo wants to merge 2 commits into
issus:masterfrom
mariioo:fix/mariusz/schlib-coordinate-units

Conversation

@mariioo

@mariioo mariioo commented Aug 31, 2026

Copy link
Copy Markdown

Correct SchLib vertex record scaling to native 10 mil DXP units. Targeted SchLib tests: 2 passed. Downstream Electronics.Altium tests: 50 passed. Full local upstream suite: 848 passed, 10 skipped, 3 unrelated GLTF loads blocked by Windows Smart App Control (0x800711C7).

@mariioo

mariioo commented Aug 31, 2026

Copy link
Copy Markdown
Author

@Sovtek33 proszę o review poprawki jednostek współrzędnych SchLib.

PR nie jest przeznaczony do merge bez osobnej decyzji po review.

@Sovtek33

Copy link
Copy Markdown

@mariioo review poprawki jednostek współrzędnych SchLib. Kierunek zmiany potwierdzam, ale nie zatwierdzam w tej wersji - trzy uwagi poniżej.

Niezależna weryfikacja u nas

  • commit: 274a419d3a46993fea9f97708e1dfdde939d1d28 (dokładnie ten, który podałeś)
  • git submodule update --init --recursive - OK
  • dotnet test tests/OriginalCircuit.Altium.Tests/OriginalCircuit.Altium.Tests.csproj -c Release --nologo - PASS: 851 zdanych, 0 niezdanych, 10 pominiętych, 28 s, net10.0

Uwaga do samego przebiegu: wśród 10 pominiętych są ReaderIntegrationTests.SchLibReader_CanReadStm32File i SchLibReader_CanReadSinglePinGndFile, czyli akurat testy czytnika SchLib na prawdziwych plikach. Zielony wynik nie jest więc dowodem na poprawność odczytu realnego SchLib po tej zmianie.

Co potwierdzam

Skala jest właściwa i spójna z resztą kodu:

  • SchLibWriter.AddCoordParam dokumentuje wprost 1 DXP = 10 mils = 100 000 raw, a czytnik używa CoordFromDxp(dxp, frac) = FromRaw(dxp * 100_000 + frac).
  • SchDocReader.TryParseCoord ma już tę samą poprawkę wraz z komentarzem, że x1000 czyniło każdy wierzchołek 100x za małym. Ta zmiana wyrównuje SchLib do SchDoc.
  • Sprawdziłem zasięg: SchLibReader.TryParseCoord jest wołany w 6 miejscach i wszystkie to X{i}/Y{i} wierzchołków. Zmiana nie rozlewa się na inne pola współrzędnych.

Uwagi blokujące

1. Test nie pokrywa tego, co naprawia PR.
Polyline_WritesNativeTenMilDxpVertexUnits sprawdza wyłącznie helper zapisu. Zmiana w czytniku (* 100_000) nie ma żadnego testu, mimo że to połowa poprawki. Test leży w SchLibRoundTripTests, ale nie wykonuje round-tripu, i ma w nazwie Polyline, choć polilinii nie tworzy. Proszę o test round-trip: zbuduj polilinię, zapisz SchLib, odczytaj i porównaj współrzędne wierzchołków - to jednocześnie pokryje czytnik i zapis.

2. Cicha utrata dokładności przy zapisie.
CoordToSchematicUnits to dzielenie całkowite raw / 100_000, obcinające w stronę zera. Po zmianie najmniejsza zapisywalna jednostka to 10 mils zamiast 0,1 mila. Wierzchołek na 105 mils zapisze się jako 10, czyli 100 mils - 5 mils błędu i po odczycie nie wraca. Ani czytnik, ani zapis nie obsługują dla wierzchołków pary X{n}_FRAC/Y{n}_FRAC (mają ją tylko LOCATION i piny). Do decyzji i do zapisania w kodzie: albo emitujemy część ułamkową, albo zaokrąglamy do najbliższej jednostki zamiast obcinać - w obu przypadkach z testem na współrzędnej spoza siatki 10 mils.

3. Dwie identyczne funkcje po zmianie.
CoordToSchematicUnits i CoordToDxpUnits mają teraz identyczne ciało. Utrzymywanie dwóch nazw na jedną skalę to dokładnie ta rozbieżność, z której wziął się ten błąd. Proponuję zostawić jedną (CoordToDxpUnits), a w SchDocWriter usunąć zbędne jawne przekazywanie konwertera i domyślny parametr vertexUnits.

Sprawy poza kodem

  • Zgadza się, że PR nie rusza .gitmodules ani gitlinka w naszym repozytorium głównym - potwierdzam po diffie (3 pliki, +15/-10).
  • Konsekwencja do zaplanowania osobno: każdy .SchLib zapisany starą wersją ma wierzchołki 100x za małe. Jeśli mamy takie artefakty z pilota STS30, po scaleniu trzeba je wygenerować od nowa. Zgłoś, proszę, czy dotyczy to plików, które już wpuściliśmy do magazynu Evidence.
  • Nie dodałeś recenzenta na PR - u nas prośba o przegląd idzie przez pole recenzenta, komentarz to treść. Przy kolejnym PR proszę o jedno i drugie.

Decyzji o merge nie podejmuję - zgodnie z Twoim zastrzeżeniem to osobny krok po review.

@mariioo

mariioo commented Sep 1, 2026

Copy link
Copy Markdown
Author

@Sovtek33 poprawiłem wszystkie trzy uwagi dla nowego HEAD
65a01f4778a605fc6d2e191db169c9a16b02cb10:

  • test wykonuje rzeczywisty SchLib write → read i porównuje wierzchołki polilinii;
  • współrzędne spoza siatki 10 mil są jawnie zaokrąglane do najbliższej jednostki,
    midpoint away from zero, z regresjami dodatnimi i ujemnymi;
  • pozostał jeden konwerter CoordToDxpUnits; usunięto duplikat i plumbing
    vertexUnits.

Weryfikacja lokalna: 855 PASS, 10 SKIP, 0 FAIL; git diff --check PASS.

Odpowiedź o starych artefaktach: binaria SchLib nie są fizycznie przechowywane w
Reviewed Evidence Store, ale stare bajty są obecne w Registry na branchach STS30/pasywów,
a Evidence wiąże odpowiadające manifesty Registry. Po przyjęciu tej poprawki trzeba
ponownie zweryfikować lub odtworzyć dotknięte SchLib i ich manifesty przed scaleniem tych
branchy; nie będę zmieniał receiptów ani gitlinka automatycznie. Gitlink głównego repo
pozostaje bez zmian do osobnego, zatwierdzonego commita po merge upstream.

Proszę o ponowny review tego exact HEAD. Próba dodania formalnego review request została
odrzucona przez GitHub z powodu braku uprawnienia właściciela forka w repo upstream, dlatego
pozostawiam jawną wzmiankę tutaj.

@Sovtek33 Sovtek33 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Przegląd ponowny dla dokładnego HEAD 65a01f4.

Wszystkie trzy uwagi zostały rozwiązane:

  • test wykonuje rzeczywisty zapis i odczyt SchLib oraz sprawdza współrzędne polilinii;
  • współrzędne poza siatką 10 mil są jawnie zaokrąglane do najbliższej jednostki, w połowie od zera, z testami dodatnimi i ujemnymi;
  • pozostał jeden wspólny konwerter CoordToDxpUnits, bez zbędnego przekazywania funkcji.

Sprawdziłem zakres 4 plików i zgodność czytnika z writerem. CI forka dla tego SHA jest zielone. Nie mam uwag blokujących.

@mariioo

mariioo commented Sep 1, 2026

Copy link
Copy Markdown
Author

@issus proszę o merge PR #55 dla dokładnego HEAD
65a01f4778a605fc6d2e191db169c9a16b02cb10.

Gitlink zostanie zaktualizowany dopiero osobno, po faktycznym merge upstream i ponownej walidacji.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants