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

Draw lines of fire when aiming vehicle turrets #154

Merged
merged 3 commits into from
Oct 29, 2020
Merged

Draw lines of fire when aiming vehicle turrets #154

merged 3 commits into from
Oct 29, 2020

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Oct 29, 2020

Summary

SUMMARY: Interface "Draw lines of fire when aiming vehicle turrets"

Purpose of change

  1. Fix exploit with automove on CURSES (cherry-picked CleverRaven#40194; required for the next change)
  2. Draw approx. lines of fire when aiming vehicle turrets to avoid confusion (CleverRaven#40198 adapted for BN)
  3. Prevent aiming cursor on TILES from disappearing when aiming at a tile out of sight (just an annoying visual bug, fixed in mainline as part of CleverRaven#39785)

Describe the solution

Cherry-pick and/or adapt code from the mainline

Describe alternatives you've considered

Cherry-picking CleverRaven#39785 for the bugfixes first, but it had accumulated a billion merge conflicts/its own changes by now. I could try to port all of that to BN and make a big PR (3k-4k lines of C++) - if you're willing to have these changes / review them?

Testing

(On CURSES) Clicked on tiles at the edge of FoV to trigger the exploit - the path shows as dark grey ? signs.

(On TILES and CURSES) Installed a bunch of turrets on a vehicle - aiming them draw individual trajectories, and pressing t toggles the display to avoid overcrowding.

(On TILES) Tried aiming with a gun at a tile outside FoV - aiming cursor remains visible

Additional context

@Coolthulhu
Copy link
Member

I could try to port all of that to BN and make a big PR (3k-4k lines of C++) - if you're willing to have these changes / review them?

If it's readable and either "obviously correct", backed up by tests, or otherwise easy to verify it works, then yes.

@Coolthulhu Coolthulhu merged commit a3eab11 into cataclysmbnteam:upload Oct 29, 2020
@olanti-p olanti-p deleted the misc-changes branch October 29, 2020 17:00
if( draw_turret_lines ) {
for( const vehicle_part *t : vturrets ) {
turret_data td = veh->turret_query( *t );
if( td.in_range( dst ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

It's a big ladder, could be more readable with some of the ifs inverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I don't normally do this, but figured I'll be rewriting this part anyway at a later point

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