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

feat(UI): add Simple and Compact compasses for small screens #6037

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Lamandus
Copy link
Contributor

@Lamandus Lamandus commented Feb 2, 2025

Purpose of change (The Why)

While the old Compass was good and informative, smaller screens (phones, tablets) had their problems of using it.

Describe the solution (The How)

Add the comp. Compass which is just a shorter regular compass. And the simple Compass, which is just one line of where Creatures are in any direction of the player.

Describe alternatives you've considered

Testing

Works as intended, code may be wonky, but that is the best effort I could muster yet.

Additional context

Checklist

grafik
Just the Compact version
grafik
Simple version
grafik

grafik
in action

Mandatory

@github-actions github-actions bot added the src changes related to source code. label Feb 2, 2025
@Lamandus Lamandus changed the title Add 2 new smaller compasses for the UI. Compact and Simple. feat: Add 2 new smaller compasses for the UI. Compact and Simple. Feb 2, 2025
@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 2, 2025

the simple version is not too important for me to implement. The compact version is more what I want to keep.

RoyalFox2140
RoyalFox2140 previously approved these changes Feb 2, 2025
Copy link
Collaborator

@RoyalFox2140 RoyalFox2140 left a comment

Choose a reason for hiding this comment

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

image

@scarf005 scarf005 changed the title feat: Add 2 new smaller compasses for the UI. Compact and Simple. feat(UI): Simple and Compact compasses for smaller screens Feb 2, 2025
@scarf005 scarf005 changed the title feat(UI): Simple and Compact compasses for smaller screens feat(UI): add Simple and Compact compasses for small screens Feb 2, 2025
@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 2, 2025

image

I used sim.Compass instead of simp.compass due to the problematic word.

@RoyalFox2140
Copy link
Collaborator

I used sim.Compass instead of simp.compass due to the problematic word.

Lol, I didn't think about that.

@OrenAudeles
Copy link
Collaborator

Using if( adx > 2 * ady ) and so on pins the diagonal wedges to between rotations of rays <2, 1> and <1, 2> which results in the cardinal directions each having around 53 degrees of coverage while the diagonal directions having around 36 degrees of coverage.
Ideally each wedge would get 45 degrees of coverage, which would be using rays along (45deg +- 22.5deg). I wrote a function direction_to_enemy_improved in godbolt online compiler using dot products and ray direction values picked from the continued fraction of cos( 22.5 deg ) / sin( 22.5 deg ).
I think your function direction_to_enemy will probably work well enough, there will just be a small cardinal bias.

@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 3, 2025

Using if( adx > 2 * ady ) and so on pins the diagonal wedges to between rotations of rays <2, 1> and <1, 2> which results in the cardinal directions each having around 53 degrees of coverage while the diagonal directions having around 36 degrees of coverage. Ideally each wedge would get 45 degrees of coverage, which would be using rays along (45deg +- 22.5deg). I wrote a function direction_to_enemy_improved in godbolt online compiler using dot products and ray direction values picked from the continued fraction of cos( 22.5 deg ) / sin( 22.5 deg ). I think your function direction_to_enemy will probably work well enough, there will just be a small cardinal bias.

Please be so kind to show me exactly how I have to do it. I am not that code savvy. 😔 Sorry

@OrenAudeles
Copy link
Collaborator

Please be so kind to show me exactly how I have to do it. I am not that code savvy. 😔 Sorry

Should be just replacing the contents of direction_to_enemy with what's in direction_to_enemy_improved. oh, and I guess make std::array<wedge_range, 8> wedges; into constexpr std::array<wedge_range, 8> wedges; I kept the function signature the same so it's a drop-in replacement :)

@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 3, 2025

grafik
works like a charm. Thank you!

Copy link
Contributor

autofix-ci bot commented Feb 3, 2025

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 3, 2025

I don't know why it throws me an error here, AI doesn't help either this time. lol

src/panels.cpp Outdated Show resolved Hide resolved
src/panels.cpp Outdated Show resolved Hide resolved
src/panels.cpp Outdated
@@ -1806,9 +1810,10 @@ std::string direction_to_enemy_improved( const tripoint &enemy_pos, const tripoi
return "--";
}

// Corrected check function
void check( const char *msg, std::function<std::string( const tripoint &, const tripoint & )> fn )
Copy link
Collaborator

@OrenAudeles OrenAudeles Feb 5, 2025

Choose a reason for hiding this comment

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

Remove the check function entirely. What I suggested was to only replace the contents of your direction_to_enemy function with the contents of direction_to_enemy_improved. All other code was for testing that the output is reasonable.

Copy link
Contributor Author

@Lamandus Lamandus Feb 5, 2025

Choose a reason for hiding this comment

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

You should have said that... 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah looking back I didn't communicate as effectively as I should have what I was suggesting. We got it fixed in the end, though, so the PR process works. "Team work makes the dream work" 😆

Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

Hopefully now it all works, will leave to the others to merge in case some code issue hasn't been addressed that I overlooked.

@Lamandus
Copy link
Contributor Author

Lamandus commented Feb 9, 2025

@scarf005 all changes are done and I still need your approval.

After that you don't have to deal with me again probably. The thumb down emojis show me that I am still not welcomed.
I just want to have a easier time on my phone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants