From cf935a2a492f78777520e9185af25fb86b370698 Mon Sep 17 00:00:00 2001 From: Bruno Postle Date: Wed, 22 Apr 2026 08:31:24 +0100 Subject: [PATCH] FIxes --- PLAN.md | 82 +++++++++++++++++++++++++++++++++++++++++++ ifcurl/service.py | 72 ++++++++++++++++++++++++++++++++----- tests/test_service.py | 36 ++++++++++++++----- 3 files changed, 173 insertions(+), 17 deletions(-) diff --git a/PLAN.md b/PLAN.md index 617547c..1ed2fb8 100644 --- a/PLAN.md +++ b/PLAN.md @@ -118,6 +118,88 @@ Add `ifc://` link detection and preview embedding to Gitea. --- +## Phase 3b — Web IFC viewer + +Add a 3D viewer that opens in a separate browser window when viewing an `.ifc` +file in Forgejo. + +### Architecture + +- **Forgejo Go patch**: when rendering the file view for a `.ifc` file, inject + a "View in 3D" button that opens + `http://localhost:8000/viewer?url=ifc://...` in a new tab. The ifc:// URL + encodes the current host, repo, ref, and file path. + +- **`/viewer` endpoint** in the ifcurl preview service: returns a self-contained + HTML page. No server-side IFC processing — the browser does all rendering. + +- **Browser rendering**: the viewer page loads `@thatopen/components` (and its + `web-ifc` + Three.js peer dependencies) from a CDN via an ES module importmap. + It constructs the Forgejo raw file URL from the ifc:// URL parameters and + fetches the IFC bytes directly from Forgejo. + +### Current scope (Phase 3b) + +- Display the full model for whatever ref is shown in Forgejo. +- Basic orbit/pan/zoom camera controls. +- No filtering or selection UI yet. + +### Future scope + +- Element filtering and selection using `@thatopen/components` classifier and + highlighter APIs. +- Camera state serialised to an `ifc://` URL so the user can copy a view link + (feeds into Phase 4 Bonsai "Copy view URL"). +- Diff highlighting driven by ifcmerge output (see Phase 3c). + +--- + +## Phase 3c — ifcmerge integration + +`ifcmerge` is a git custom merge driver that resolves conflicts in `.ifc` files +that git's built-in 3-way merge cannot handle. Where two branches have both +modified an IFC file (a fork), git would report an unresolvable conflict; +ifcmerge performs a semantics-aware merge and produces a valid merged IFC file. + +### Git merge driver configuration + +Register ifcmerge as the git merge driver for `.ifc` files on the Forgejo +server: + +``` +# /etc/gitconfig or ~forgejo/.gitconfig +[merge "ifcmerge"] + name = IFC merge driver + driver = ifcmerge %O %A %B %L +``` + +``` +# .gitattributes in each repo, or server-side gitattributes +*.ifc merge=ifcmerge +``` + +With this in place, Forgejo's existing "merge automatically" path will invoke +ifcmerge via git when merging `.ifc` files, and the PR merge button will work +without any Go-side changes for the common case. + +### Forgejo integration + +- Verify that Forgejo's merge-ability check (the "these branches can be merged + automatically" indicator) correctly reflects ifcmerge's ability to merge + `.ifc` files — investigate whether the check runs a trial merge via git or + uses a heuristic. +- If needed, patch Forgejo to run a trial `git merge --no-commit` to get an + accurate result for IFC files. + +### 3D diff view (future) + +- After a merge completes, surface a "View merge in 3D" button in the PR using + the Phase 3b viewer. +- Use `@thatopen/components` highlighter to colour added, removed, and modified + elements differently, driven by comparing the merged IFC against either parent. + +--- + ## Phase 4 — Bonsai integration Add `ifc://` handling to Bonsai. diff --git a/ifcurl/service.py b/ifcurl/service.py index 500cf5f..4d0576e 100644 --- a/ifcurl/service.py +++ b/ifcurl/service.py @@ -39,6 +39,7 @@ import hashlib import os import tempfile import threading +import time from collections import OrderedDict from pathlib import Path @@ -128,11 +129,11 @@ def _guids_to_step_ids(model: ifcopenshell.file, guids: frozenset[str]) -> list[ # --------------------------------------------------------------------------- -# Tier 4: sha256(url) → PNG (filesystem, immutable refs only) +# Tier 4: sha256(url) → PNG (filesystem, immutable refs only, no expiry) # --------------------------------------------------------------------------- def _t4_path(url: str) -> Path: - cache_dir = Path(user_cache_dir("ifcurl")) / "renders" + cache_dir = Path(user_cache_dir("ifcurl")) / "renders" / "immutable" cache_dir.mkdir(parents=True, exist_ok=True) url_hash = hashlib.sha256(url.encode()).hexdigest() return cache_dir / f"{url_hash}.png" @@ -149,6 +150,36 @@ def _t4_put(url: str, png: bytes) -> None: _t4_path(url).write_bytes(png) +# --------------------------------------------------------------------------- +# Tier 4m: sha256(url) → PNG (filesystem, mutable refs, 5-minute TTL) +# --------------------------------------------------------------------------- + +_T4M_TTL = 300 # seconds + + +def _t4m_path(url: str) -> Path: + cache_dir = Path(user_cache_dir("ifcurl")) / "renders" / "mutable" + cache_dir.mkdir(parents=True, exist_ok=True) + url_hash = hashlib.sha256(url.encode()).hexdigest() + return cache_dir / f"{url_hash}.png" + + +def _t4m_get(url: str) -> tuple[bytes, int] | None: + """Return (png_bytes, remaining_seconds) or None if missing/expired.""" + path = _t4m_path(url) + try: + remaining = int(os.path.getmtime(path) + _T4M_TTL - time.time()) + if remaining <= 0: + return None + return path.read_bytes(), remaining + except (FileNotFoundError, OSError): + return None + + +def _t4m_put(url: str, png: bytes) -> None: + _t4m_path(url).write_bytes(png) + + # --------------------------------------------------------------------------- # Helper: load model from bytes via a temp file # --------------------------------------------------------------------------- @@ -213,11 +244,24 @@ def preview(request: PreviewRequest) -> Response: if ifc_url.path is None: raise HTTPException(status_code=400, detail="URL has no 'path' parameter") - # --- Tier 4: cached PNG for immutable refs --- - if not ifc_url.is_mutable_ref(): + # --- Tier 4 / 4m: cached PNG --- + if ifc_url.is_mutable_ref(): + t4m_hit = _t4m_get(request.url) + if t4m_hit is not None: + cached_png, remaining = t4m_hit + return Response( + content=cached_png, + media_type="image/png", + headers={"Cache-Control": f"public, max-age={remaining}"}, + ) + else: cached_png = _t4_get(request.url) if cached_png is not None: - return Response(content=cached_png, media_type="image/png") + return Response( + content=cached_png, + media_type="image/png", + headers={"Cache-Control": "public, max-age=31536000, immutable"}, + ) # --- Resolve authentication token --- token = request.token @@ -284,8 +328,18 @@ def preview(request: PreviewRequest) -> Response: except ValueError as exc: raise HTTPException(status_code=422, detail=str(exc)) from exc - # --- Tier 4: store PNG for immutable refs --- - if not ifc_url.is_mutable_ref(): + # --- Tier 4 / 4m: store PNG and return with appropriate cache headers --- + if ifc_url.is_mutable_ref(): + _t4m_put(request.url, png_bytes) + return Response( + content=png_bytes, + media_type="image/png", + headers={"Cache-Control": f"public, max-age={_T4M_TTL}"}, + ) + else: _t4_put(request.url, png_bytes) - - return Response(content=png_bytes, media_type="image/png") + return Response( + content=png_bytes, + media_type="image/png", + headers={"Cache-Control": "public, max-age=31536000, immutable"}, + ) diff --git a/tests/test_service.py b/tests/test_service.py index db0534e..571b056 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -24,8 +24,9 @@ SELECTOR_URL = "ifc://example.com/org/repo@heads/main?path=model.ifc&selector=If @pytest.fixture(autouse=True) -def clear_caches(): - """Reset in-memory caches and tier-4 filesystem cache between tests.""" +def clear_caches(tmp_path, monkeypatch): + """Reset in-memory caches and redirect filesystem PNG cache between tests.""" + monkeypatch.setattr("ifcurl.service.user_cache_dir", lambda *a, **kw: str(tmp_path)) _t2_cache.clear() _t3_cache.clear() yield @@ -34,9 +35,8 @@ def clear_caches(): @pytest.fixture() -def tmp_t4_cache(tmp_path, monkeypatch): - """Redirect tier-4 PNG cache to a temp directory.""" - monkeypatch.setattr("ifcurl.service.user_cache_dir", lambda *a, **kw: str(tmp_path)) +def tmp_t4_cache(tmp_path): + """Return the temp cache directory (already redirected by clear_caches).""" return tmp_path @@ -109,7 +109,7 @@ class TestTier2Cache: client.post("/preview", json={"url": MUTABLE_URL}) assert (FAKE_HEXSHA, "model.ifc") in _t2_cache - def test_second_request_uses_cached_bytes(self, model_with_geometry): + def test_second_request_uses_cached_bytes(self, model_with_geometry, monkeypatch): ifc_bytes = model_with_geometry.to_string().encode() call_count = 0 @@ -118,6 +118,9 @@ class TestTier2Cache: call_count += 1 return FAKE_HEXSHA, ifc_bytes + # Disable tier-4m so both requests reach fetch_ifc, letting us + # verify that tier-2 serves ifc_bytes from cache on the second call. + monkeypatch.setattr("ifcurl.service._T4M_TTL", 0) with patch("ifcurl.service.fetch_ifc", counting_fetch), \ patch("ifcurl.service.render_mod.render", return_value=FAKE_PNG): client.post("/preview", json={"url": MUTABLE_URL}) @@ -192,7 +195,7 @@ class TestTier4Cache: assert r2.content == FAKE_PNG assert render_call_count == 1 # render only called once - def test_png_not_cached_for_mutable_ref(self, tmp_t4_cache, model_with_geometry): + def test_png_cached_for_mutable_ref_within_ttl(self, tmp_t4_cache, model_with_geometry): ifc_bytes = model_with_geometry.to_string().encode() render_call_count = 0 @@ -206,7 +209,24 @@ class TestTier4Cache: client.post("/preview", json={"url": MUTABLE_URL}) client.post("/preview", json={"url": MUTABLE_URL}) - assert render_call_count == 2 # no tier-4 cache for mutable ref + assert render_call_count == 1 # second request served from tier-4m cache + + def test_png_not_cached_for_mutable_ref_after_ttl(self, tmp_t4_cache, model_with_geometry, monkeypatch): + ifc_bytes = model_with_geometry.to_string().encode() + render_call_count = 0 + + def counting_render(*args, **kwargs): + nonlocal render_call_count + render_call_count += 1 + return FAKE_PNG + + monkeypatch.setattr("ifcurl.service._T4M_TTL", 0) + with patch("ifcurl.service.fetch_ifc", _mock_fetch(ifc_bytes)), \ + patch("ifcurl.service.render_mod.render", counting_render): + client.post("/preview", json={"url": MUTABLE_URL}) + client.post("/preview", json={"url": MUTABLE_URL}) + + assert render_call_count == 2 # TTL=0 means cache always expired def test_tier4_cache_file_exists_on_disk(self, tmp_t4_cache, model_with_geometry): ifc_bytes = model_with_geometry.to_string().encode()