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

OcdFileImport: Revise path coord processing #2224

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

Conversation

dg0yt
Copy link
Member

@dg0yt dg0yt commented Apr 14, 2024

Resolves #2144: Importing accepting pairs of hole points, but editor crashing when selecting the export.

This PR moves some single-use helper functions back into the processing loops. This allows optimizing the conditions and adding the desired fix. (The original functions might have been more easy to analyze separately, but passing and modifying pos in the call stack added complexity.)

The PR is broken into a series of commits which allow to track the source code transformation. There is no specific unit test to cover this change.

@dg0yt dg0yt mentioned this pull request Apr 14, 2024
@dl3sdo dl3sdo self-assigned this Jul 28, 2024
@dl3sdo dl3sdo self-requested a review July 28, 2024 18:32
@dl3sdo
Copy link
Member

dl3sdo commented Jul 28, 2024

I'm currently writing an unit test.

@dl3sdo
Copy link
Member

dl3sdo commented Jul 30, 2024

Kai, I had to rebase your PR to base it on the latest version of file_format_t.
I was not sure whether I should force-push to your branch, so I pushed to https://github.com/dl3sdo/mapper/tree/ocd-hole-points

Please check whether the unit test goes into the right direction.

I'm currently only testing area objects and I don't test all the flags. Although I crafted the .ocd coords to be real coordinates I intentionally don't check the x- and y-values of the coordinates after conversion.

@dl3sdo dl3sdo removed their assignment Jul 30, 2024
@dg0yt
Copy link
Member Author

dg0yt commented Jul 31, 2024

https://github.com/dl3sdo/mapper/tree/ocd-hole-points

I will pick the commits which add the actual test and work on it separately first.

@dg0yt
Copy link
Member Author

dg0yt commented Jul 31, 2024

Moving to this test data pattern:

	QTest::addColumn<OcdPointsView>("data");
	QTest::addColumn<FlagsView>("expected");
	
	{
		static Ocd::OcdPoint32 ocd_points[] = {
		    // +0,  bezier
		    { C(-1109), C(212) },
		    { C(-1035) | Ocd::OcdPoint32::FlagCtl1, C(302) },
		    { C(-1008) | Ocd::OcdPoint32::FlagCtl2, C(519) },
		    { C(-926), C(437) },
		};
		static int expected_flags[] = {
		    MapCoord::CurveStart, 0, 0, 0, MapCoord::ClosePoint,
		};
		QTest::newRow("bezier") << OcdPointsView(ocd_points) << FlagsView(expected_flags);
	}

Those view classes are litte more than [ size, pointer ].

}
return ++i;
});
Q_ASSERT(object->coords.size() >= count);
Copy link
Member

@dl3sdo dl3sdo Aug 2, 2024

Choose a reason for hiding this comment

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

Q_ASSERT(object->coords.size() >= static_cast<std::size_t>(count));

Preparation for improved state handling and bug fixes.
No observable change of behavior intented.
Preparation for improved state handling and bug fixes.
No observable change of behavior intented.
@dg0yt dg0yt marked this pull request as draft August 3, 2024 13:57
@dg0yt
Copy link
Member Author

dg0yt commented Aug 3, 2024

Plan:

  • Test behavior-preserving changes
  • Adjust test to desired changes of behavior
  • Test changes

... and use iterators for input and output.

Preparation for improved state handling and bug fixes:
No tight coupling of input position and output position.
No observable change of behavior intented.
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