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

TESTS: Add unit test for Engines::Trigger class #372

Closed
wants to merge 9 commits into from

Conversation

rjshae
Copy link
Contributor

@rjshae rjshae commented Sep 23, 2018

This test defines a utility Trigger class that can accept an array of points in
X-Y-Z space. These are pushed onto the classes' _geometry vector for use with
the contains(float x, float y) function.

Each test provides a static array of points to the class that define a non-convex,
closed polygon -- equivalent to an in-game trigger region. The class contains
function is then passed a series of test points, and the output is compared to
the expected results.

For clarity, in tests/engines/trigger.cpp the comments for each test include an
ASCII illustration demonstrating the shape of the polygon and the location of
the test points.

@rjshae
Copy link
Contributor Author

rjshae commented Sep 23, 2018

All four tests should fail because the contains call is returning unexpected results.
test-suite.log

@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 23, 2018

Huh. So our Trigger class is wrong? Good to know. Thanks for finding this! :)

I don't want to merge failing tests, but this would be a good starting point to fix the code. Can you fix it, and add that to this PR? And move the fixing commits before the test.

Also: c9b976c, b64b62f and 0e8ee66 shouldn't be their own commit, but they should be squashed into the commit 9d9b7b4. So that in the end, it looks like one commit that does the "right" thing from the start.

Also also, to fix up a PR, you don't need to close it and open a new one, like you did with #369, #370 and #371. Instead, force-push to your remote branch that you used to base the PR off of. That will update the PR.

@rjshae
Copy link
Contributor Author

rjshae commented Sep 23, 2018 via email

@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 23, 2018

although I have no idea how to move commits around like that

You can do that with an interactive rebase. I.e. "git rebase -i", and the commit ID of the point where you're starting modifying the history.

Each line there is a single commit, if you move it up and down the stack, you're reordering the commits. If they touch different files/code, that should work cleanly. Otherwise, you'll have to manually fix up each conflict, and that's sometimes a bit hairy.

@rjshae
Copy link
Contributor Author

rjshae commented Sep 25, 2018

I've updated src/engines/aurora/trigger.cpp to work properly with an updated unit test and revised the commit history accordingly. The revision was force pushed to rjshae/xoreos and I can see the commits there, but this pull request hasn't updated. Any suggestions?

@DrMcCoy
Copy link
Member

DrMcCoy commented Sep 25, 2018

@rjshae
Copy link
Contributor Author

rjshae commented Sep 25, 2018 via email

@rjshae rjshae force-pushed the xoreos branch 2 times, most recently from cd89ffd to a58e601 Compare October 2, 2018 00:46
@rjshae
Copy link
Contributor Author

rjshae commented Oct 2, 2018

I just fixed up the include file spacing.

This revision (slightly) improves the performance of
the ray-casting algorithm by precomputing the slopes
of the trigger polygon sides.
This test will pass an array of points to a utility Trigger
class that define a close, non-convex polygon. The class'
'contains(float x, float y)' function is called for a set
of test points. If the function is working properly, it will
return the expected boolean result declared in the test.

See the tests/engines/trigger.cpp comments for illustrations.
@DrMcCoy
Copy link
Member

DrMcCoy commented Nov 3, 2018

Merged as 29ce633...cda3687, thanks! :)

@DrMcCoy DrMcCoy closed this Nov 3, 2018
@rjshae rjshae deleted the xoreos branch February 5, 2019 05:27
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.

None yet

2 participants