lrs: expose planar coordinate system #86

Castavo · 2024-10-25T12:04:07Z

This is a first approach, I don't know if it's good enough.

The choice of having a planar argument for build and load doesn't convince me completely, what do you think ? (beside that it could be an enum)

Signed-off-by: Baptiste Prevot <[email protected]>

Tristramg · 2024-10-26T13:00:27Z

src/lrs_ext.rs

-        Lrs::from_bytes(data)
-            .map(|lrs| Self { lrs })
-            .map_err(|err| err.to_string())
+    pub fn load(data: &[u8], planar: bool) -> Result<ExtLrs, String> {


The serialized data stores what kind of coordinates we are dealing with: https://github.com/OpenRailAssociation/liblrs/blob/main/schema/lrs.fbs#L8 (from the wording, we can see that it was a different use case that was initially though of, but it still work).

This will however draw a lot of small issues. I’m not sure how we could get a nice overview of all the fiddling we’ll have to do

I don't really see the issues, could you elaborate ?

Is it that

we need to set it through the builder (which my PR doesn't do, I reckon)

we need to read it when deserializing (which my PR doesn't do either)

It’s both.

load shouldn’t take a parameter as it’s defined in the datafile. Loading planar graph as spherical would result in unexpected results (plus the more complicated API).

Castavo · 2024-10-28T09:27:35Z

Also @Tristramg, what do you think of the method of wrapping it in an enum ?

SergeCroise · 2024-10-28T13:26:43Z

python/src/lib.rs

@@ -321,24 +329,17 @@ impl Lrs {

    /// Given a ID returns the corresponding lrs index (or None if not found)


Suggested change

/// Given a ID returns the corresponding lrs index (or None if not found)

/// Given an ID, returns the corresponding lrs index (or None if not found)

SergeCroise · 2024-10-28T13:30:30Z

src/lrs_ext.rs

+        }
+    }
+
+    /// Given a ID returns the corresponding lrs index (or None if not found)


Suggested change

/// Given a ID returns the corresponding lrs index (or None if not found)

/// Given an ID, returns the corresponding lrs index (or None if not found)

SergeCroise · 2024-10-28T13:31:41Z

src/lrs_ext.rs

+        }
+    }
+
+    /// Get the positon along the curve given a [`LrmScaleMeasure`]


a [`LrmScaleMeasure`] or an [`LrmScaleMeasure`]

Tristramg · 2024-10-28T13:32:15Z

I think wrapping with an enum is very nice. The API of ExtLrs stays simple to use as it does the dispatch as required (maybe a macro could simplify the internal code, but I’m just thinking out loud, and I’m not sure if it would help much).

The only problem is how to handle this at Lrs level where new and from_bytes return Self. Maybe they could return the enum? I’m not sure how it would behave

SergeCroise · 2024-10-28T13:54:03Z

wasm/src/lib.rs

@@ -1,10 +1,7 @@
 //! High level extensions meant for an easy usage
 //! Those functions are exposed in wasm-bindings


`wasm-bindings` or [`wasm-bindings`]

Castavo · 2024-10-29T20:09:24Z

Upon further thinking, maybe the enum as I did it resolves the symptoms and not the problem.

Maybe Lrs shouldn't be generic on Curve, it actually bugged me when first implementing this feature. Indeed: whether the Lrs actually has spherical or planar curves has little impact on how we handle the Lrs later: to compute distances, or get geometry ranges, it feels like we don't really need to know what's happening under the hood.

Different ideas to not have the Curve genericity in Lrs:

Have something along the lines of traversals: Vec<Traversal<dyn Curve>> (or impl Cuvre ?) (I have no idea what this implies, I don't know rust enough)

Have a TraversalCollection enum which would have two variants

enum TraversalCollection {
    Spherical(Vec<Traversal<SphericalLineStringCurve>>)
    Planar(Vec<Traversal<PlanarLineStringCurve>>)

Expand the coverage of methods in LrsBase and use a dyn LrsBase inside ExtLrs (also, maybe ExtLrs could be something more explicit than a wrapper, like a Trait implemented on LrsBase or something)
Maybe even remove the Genericity from the traversals too ?

Castavo · 2024-10-30T10:13:23Z

I could also keep with the Enum, implement the two things you talked about (I think it's actually quite straightforward) and we'll make it cleaner later

lrs: expose planar coordinate system

6d8bd3e

Signed-off-by: Baptiste Prevot <[email protected]>

Castavo force-pushed the expose-planar branch from e541bde to 6d8bd3e Compare October 25, 2024 12:11

Tristramg reviewed Oct 26, 2024

View reviewed changes

SergeCroise reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lrs: expose planar coordinate system #86

lrs: expose planar coordinate system #86

Castavo commented Oct 25, 2024 •

edited

Loading

Tristramg Oct 26, 2024

Castavo Oct 28, 2024

Tristramg Oct 28, 2024

Castavo commented Oct 28, 2024

SergeCroise Oct 28, 2024

SergeCroise Oct 28, 2024

SergeCroise Oct 28, 2024 •

edited

Loading

Tristramg commented Oct 28, 2024

SergeCroise Oct 28, 2024 •

edited

Loading

Castavo commented Oct 29, 2024 •

edited

Loading

Castavo commented Oct 30, 2024

		@@ -321,24 +329,17 @@ impl Lrs {

		/// Given a ID returns the corresponding lrs index (or None if not found)

	/// Given a ID returns the corresponding lrs index (or None if not found)
	/// Given an ID, returns the corresponding lrs index (or None if not found)

		@@ -1,10 +1,7 @@
		//! High level extensions meant for an easy usage
		//! Those functions are exposed in wasm-bindings

lrs: expose planar coordinate system #86

Are you sure you want to change the base?

lrs: expose planar coordinate system #86

Conversation

Castavo commented Oct 25, 2024 • edited Loading

Tristramg Oct 26, 2024

Choose a reason for hiding this comment

Castavo Oct 28, 2024

Choose a reason for hiding this comment

Tristramg Oct 28, 2024

Choose a reason for hiding this comment

Castavo commented Oct 28, 2024

SergeCroise Oct 28, 2024

Choose a reason for hiding this comment

SergeCroise Oct 28, 2024

Choose a reason for hiding this comment

SergeCroise Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Tristramg commented Oct 28, 2024

SergeCroise Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Castavo commented Oct 29, 2024 • edited Loading

Castavo commented Oct 30, 2024

Castavo commented Oct 25, 2024 •

edited

Loading

SergeCroise Oct 28, 2024 •

edited

Loading

SergeCroise Oct 28, 2024 •

edited

Loading

Castavo commented Oct 29, 2024 •

edited

Loading