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

Move Field of Fire Sprites out of BoardView #5466

Merged
merged 17 commits into from
May 19, 2024

Conversation

SJuliez
Copy link
Member

@SJuliez SJuliez commented May 12, 2024

This PR builds on the previous PRs and includes their changes so it would be wasteful to review it before the other stuff is merged and the amount of changes is reduced. I'll probably have to re-upload it then.

This PR moves the handling of field of fire sprites out of BoardView into its own handler that is no longer a mandatory part of the BoardView. Managing that handler is done in ClientGUI only. The *Displays call ClientGUI methods to pass on changes in the field of fire such as position, facing etc.

}


public Map<Integer, List<Report>> createFilteredReport(Player recipient) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'recipient' is never used.
Copy link
Member Author

Choose a reason for hiding this comment

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

preliminary code (recipient will be needed at some point)

private final GameListener gameListener = new SBFClientGUIGameListener(this);
private final CommonMenuBar menuBar = CommonMenuBar.getMenuBarForGame();

public SBFClientGUI(SBFClient client, MegaMekController c) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'c' is never used.
Copy link
Member Author

Choose a reason for hiding this comment

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

preliminary code

private static final String FILENAME_ICON_32X32 = "megamek-icon-32x32.png";
private static final String FILENAME_ICON_48X48 = "megamek-icon-48x48.png";
private static final String FILENAME_ICON_256X256 = "megamek-icon-256x256.png";
public class ClientGUI extends AbstractClientGUI implements BoardViewListener,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is so much cleaner - nice work.

import java.util.Set;

/**
* This BoardViewSpriteHandler handles the sprites for the firing arcs (field of fire) that can be shown for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for putting "firing arc" in the comments; "field of fire" is not intuitive to me and I'm constantly struggling to find this kind of code because of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was having trouble finding the GUIP setting myself :D And I added that whole feature originally (was merged at the time by arlith as I had no clue about PRs. Ten years ago)

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure it actually predated GitHub when we where on Sourceforge and you had to do patches via Subversion. You're patch generated a lot of buzz in the dev team at that time.

Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

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

Just a few questions and a couple of lines that were commented out.
It would also be good to attend to the codeQL issues.

Would it be possible to add some more info about these changes to the commit summary?
Specifically, the addition of some SBF and Alpha Strike functionality goes well beyond moving Field of Fire sprites, and should probably be at least mentioned.

Otherwise, looks good!

@SJuliez
Copy link
Member Author

SJuliez commented May 16, 2024

Would it be possible to add some more info about these changes to the commit summary?
Specifically, the addition of some SBF and Alpha Strike functionality goes well beyond moving Field of Fire sprites, and should probably be at least mentioned.

This is why I wrote in the first post that this would best be reviewed after merging the previous PRs because this builds on them. After merging previous PRs and updating this one it will become much smaller :)
I don't know how to make a PR that only shows changes relative to another PR. I just notice that in my list of local branches I lose sight of what else I wanted to make a PR of when I dont do it immediately. And I find it difficult to make these changes side-by-side instead of building upon each other.

@SJuliez
Copy link
Member Author

SJuliez commented May 16, 2024

It is not my intention to create useless effort. I should have made this one a draft. Mea culpa

# Conflicts:
#	megamek/src/megamek/client/ui/swing/ClientGUI.java
#	megamek/src/megamek/common/strategicBattleSystems/SBFGame.java
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 0.19157% with 521 lines in your changes are missing coverage. Please review.

Project coverage is 29.27%. Comparing base (d3202f0) to head (55e2bae).
Report is 54 commits behind head on master.

Current head 55e2bae differs from pull request most recent head 637ce46

Please upload reports for the commit 637ce46 to get more accurate results.

Files Patch % Lines
...ent/ui/swing/boardview/FiringArcSpriteHandler.java 0.00% 166 Missing ⚠️
...amek/src/megamek/client/ui/swing/SBFClientGUI.java 0.00% 130 Missing ⚠️
megamek/src/megamek/client/ui/swing/ClientGUI.java 0.00% 53 Missing ⚠️
.../megamek/client/ui/swing/AbstractPhaseDisplay.java 0.00% 40 Missing ⚠️
...c/megamek/client/ui/swing/BoardViewsContainer.java 0.00% 28 Missing ⚠️
...megamek/client/ui/swing/StartingScenarioPanel.java 0.00% 13 Missing ⚠️
megamek/src/megamek/client/AbstractClient.java 0.00% 12 Missing ⚠️
...k/src/megamek/client/ui/swing/MovementDisplay.java 0.00% 11 Missing ⚠️
megamek/src/megamek/common/Mounted.java 0.00% 11 Missing ⚠️
...src/megamek/client/ui/swing/AbstractClientGUI.java 0.00% 7 Missing ⚠️
... and 18 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5466      +/-   ##
============================================
- Coverage     29.27%   29.27%   -0.01%     
- Complexity    13650    13686      +36     
============================================
  Files          2434     2440       +6     
  Lines        261580   261813     +233     
  Branches      46676    46684       +8     
============================================
+ Hits          76588    76646      +58     
- Misses       181167   181350     +183     
+ Partials       3825     3817       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sleet01
Copy link
Collaborator

Sleet01 commented May 19, 2024

@SJuliez @HammerGS I just saw the review changes; looks good to go!

@HammerGS HammerGS merged commit 5d69556 into MegaMek:master May 19, 2024
5 checks passed
HammerGS added a commit that referenced this pull request May 19, 2024
+ PR #5487: Accessibility window cleanup and safety
+ Fix 5482: Process weapon quirks correctly in loading/saving protomeks #5484
+ PR #5481: Name Changes Updates
+ PR #5466: Move Field of Fire Sprites out of BoardView
+ PR #5472: Use FlatLaf exclusively
@SJuliez SJuliez deleted the clientgui-boardviews-2 branch May 19, 2024 20:25
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.

3 participants