Repository navigation
Give feature pointers a long-name type and add lookups by long name - #35
Merged
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A feature-to-feature pointer (FFPT) names its target only by long name (AGEN, FIDN, FIDS). Until now it was stored as an
S57RecordNamewith RCNM and RCID left at zero. That had three consequences:ReferencingFeatureswas keyed by names that no feature had.int.This PR changes that:
S57LongNametype. Areadonly record structholdingAgencyCode,FeatureId(uint) andFeatureSubdivision.S57FeaturePointer.Nameis now anS57LongName.LongNameproperty onS57RecordName,S57FeatureRecordandS57TypedFeature. It isnullwhen a record has no FOID field.S57Chart.FeaturesByLongName,GetFeature(S57LongName)andGetReferencingFeatures(S57LongName).ReferencingFeaturesis now keyed by long name.S57Document.GetFeatureRecord(S57LongName). It scans every record, while the chart's lookup uses an index.docs/reading-s57-charts.mdon following relationships between features, and a note in thes57-encskill.Memory cost. The long-name index is a read-only
Dictionaryinstead of anImmutableDictionary. WithImmutableDictionary, building the chart allocated 5.4% more and the chart retained 4.8% more. WithDictionary, the chart's retained memory went back inside its tolerance, and building it allocates 3.2% more. I raised theS57ChartFromDocumentandS57ChartRetainedbudgets to the measured values: 1,063,752 and 1,178,960 bytes.The reader's output is unchanged.
GetFeature.Standards alignment
iso8211)s57-enc)chart-viewer)Standard section references cited in code or docs:
S-57 Part 3, 4.3 (feature object identifier) and 4.6 (feature-to-feature relationships), on
S57LongNameandS57FeaturePointer.Name.Tests
tests/. New tests cover:S57LongName, including a FIDN aboveint.MaxValue;GetFeatureRecord(S57LongName);dotnet test --configuration Releasepasses locally. Unit tests: 528 pass. Budget tests: 14 pass.Documentation
src/<project>/README.md. Not needed: it doesn't cover feature pointers.docs/if user-facing behavior changeddocfx docfx.json --warningsAsErrorspasses, if docs changedDependencies
Directory.Packages.props(not in the.csproj)gh-advisory-databasesecurity check run for any new dependency. Not applicable: no new dependencies.Breaking changes
These are binary and source breaks to EncDotNet.S57, recorded in
src/EncDotNet.S57/CompatibilitySuppressions.xmlfor 0.8.0:S57FeaturePointer.NameisS57LongName(wasS57RecordName).S57Chart.ReferencingFeaturesisIReadOnlyDictionary<S57LongName, IReadOnlyList<S57TypedFeature>>(was keyed byS57RecordName).Impact on EncDotNet.S100:
S57ToS101TranslatorcallsLnam(fp.Name)in four places. TheLnam(S57RecordName)helper needs anS57LongNameoverload, or a change of parameter type. Its(ushort)feat.RecordName.AgencyCode/(uint)…FeatureIdcasts can usefeat.LongNameinstead, but don't have to.🤖 Generated with Claude Code
https://claude.ai/code/session_01YagvELyJKBANk61DyFGUNJ
Generated by Claude Code