Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LineSymbol: Revise CenterOfGap symbols #2325

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dg0yt
Copy link
Member

@dg0yt dg0yt commented Jan 24, 2025

For GH-2324.
No change necessary or implemented for the intented use case, which is symbols which fit into the gap between the dashes.
For symbols outside the gap, this PR avoids undefined rendering behavior signaled by an assert in SplitPathCoord:at by controlling rendering based symbols being outside (omitted) or inside (rendered) of the [start,end] range passed into createDashGroups. This interval is a moving window over start, explicit dash points, and end of a path.
The new behavior also creates consistent mid symbol positions regardless of path direction.

test/path_object_t.cpp Outdated Show resolved Hide resolved
@dg0yt
Copy link
Member Author

dg0yt commented Jan 24, 2025

It would probably be best to degrade gracefully for cases before the search start.

But cases outside the path are tricky: You could reasonably argue to force them into the valid range. OTOH this may break loops which are naively limited by reaching a desired length.

src/core/path_coord.cpp Outdated Show resolved Hide resolved
@dg0yt
Copy link
Member Author

dg0yt commented Feb 2, 2025

This PR now includes a test for actual rendering.
The test of SplitPathCoord::at will be in a different PR, connected with different behavioral changes.

@dg0yt dg0yt marked this pull request as ready for review February 2, 2025 07:16
@dg0yt dg0yt changed the title Test and revise SplitPathCoord::at LineSymbol: Revise CenterOfGap symbols Feb 2, 2025
dg0yt added 3 commits February 2, 2025 09:57
Eplicitly omit all symbols outside start/end.
Resolves failing assert() on some curves.
Establishes equal rendering for straight and curved case.
Avoid unequal spacing but may output less symbols.
@dg0yt dg0yt marked this pull request as draft February 2, 2025 08:57
@dg0yt
Copy link
Member Author

dg0yt commented Feb 2, 2025

Reduced this PR to changing only linesymbol, i.e. the calling site triggering the particular assert. Changes to, and tests of, SplitPathCoord::at are more sensitive and go to a different PR.

Comment on lines +1243 to +1247
auto split = (position >= dash_start.clen) ? dash_start : start;
for (int i = 0; i < mid_symbols_per_spot; ++i, position += mid_symbol_distance_f)
{
if (position < start.clen)
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

Key changes for avoiding the assert.

Comment on lines +1250 to +1251
if (position > end.clen)
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

For symmetry. Noticable when changing the direction of the path.

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.

1 participant