Skip to content

Make various refactors to clean up codebase #1830

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

Merged
merged 21 commits into from
Apr 20, 2025
Merged

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Mar 20, 2025

Description

Code that was unused has been removed to tidy up the codebase.
The creation of arrays was also tidied up, using initializers when possible and just new Type[0] for zero length arrays.
Text blocks were used to increase readability.
VideoSimUtil was refactored to reduce copies when generating 36h11 tag images.
FileSaveFrameConsumer was refactored to let Java handle paths instead of us doing it manually.
I switched to WPILib's Pair class instead of the Apache Commons Pair class. This allowed us to drop a few Apache Commons dependencies, which is good for reducing JAR size and the number of things we need to pull in.
#1556 added a UiVmmState class, but it appears to have been unused, so removing it to reduce confusion.
printTestUtils was a common function duplicated across several tests, so they were consolidated into TestUtils for maintainability.
blacklistedResIndices was removed from HardwareConfig because the filtering logic that used it was broken. Matt says it was something from the mmal days, but we use libcamera now, so we just don't need it.
collect(Collectors.toList()) for Streams was replaced with toList() when the resulting List wasn't modified. It's more concise and possibly a little bit more performant.
Math.hypot was used in place of Math.sqrt(a^2 + b^2) for conciseness.
Several classes were made into records to reduce boilerplate and increase readability.
In a few places, especially in pipelines, objects were being assigned to a variable, only to be passed to a method and never used again in the same scope. Worse, this wasn't consistent, and assignment was just skipped entirely sometimes. Now, the code more consistently creates the object directly in the method call, skipping the unnecessary assignment.
switch case and instanceof use was refactored to reduce unnecessary operations like casting if statements and casts and improve code clarity.
JUnit assertions were not always statically imported, so I statically imported all of them.
A Timer is now used to calculate FPS instead of handrolling the time tracking for readability and maintainability.
Whatever CameraServer bug caused #123 seems to have been fixed, so CameraServer is now used directly instead of duplicating its code.
QuirkyCamera was refactored to use EnumMap, since that seems to be more efficient. This also enables more concise copying of QuirkyCamera in getQuirkyCamera by just passing in the Map of the QuirkyCamera. Also, putAll was used instead of manually copying quirks in updateQuirks for conciseness.
There's some other miscellanous clean up in Miscellanous clean up, which is mostly either removing unnecessary code or making operations more efficient.

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with settings back to v2024.3.1
  • If this PR addresses a bug, a regression test for it is added

@Gold856 Gold856 requested a review from a team as a code owner March 20, 2025 05:39
@mcm001
Copy link
Contributor

mcm001 commented Mar 21, 2025

Do we wanna hold off until champs on this one?

@Gold856
Copy link
Contributor Author

Gold856 commented Mar 21, 2025

I’d be fine with that. I think there are other places I could do refactors as well.

Gold856 added 2 commits March 30, 2025 00:15
Matt says this was for mmal, and we use libcamera now, so this is unnecessary now. Also, the filtering logic that used blacklistedResIndices was completely broken.
@Gold856 Gold856 requested a review from a team as a code owner March 30, 2025 05:13
@Gold856 Gold856 force-pushed the refactor branch 2 times, most recently from a5f8e63 to 76f6528 Compare March 30, 2025 06:04
Gold856 added 3 commits March 30, 2025 20:25
In places like pipelines or DataSocketHandler, objects were being instantiated and assigned to a variable, but were then passed into a method, never to be used again. For pipelines especially, that style of code wasn't always consistently used, and other pipelines skipped assignment and instantiated objects in the method call. Make everything consistent by always instantiating in the method call. GenericUSBCameraSettables also had an unnecessary assignment, and that was cleaned up too for readability.
@Gold856 Gold856 force-pushed the refactor branch 3 times, most recently from 6aba6f7 to 7175275 Compare April 7, 2025 04:51
samfreund
samfreund previously approved these changes Apr 11, 2025
Copy link
Contributor

@mcm001 mcm001 left a comment

Choose a reason for hiding this comment

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

Blocking review until post champs

@samfreund samfreund added the enhancement New feature or request label Apr 11, 2025
@samfreund
Copy link
Member

public static final String HW_CFG_FNAME = "hardwareConfig.json";

Take a peek at these guys, they don't seem to be doing anything

@mcm001
Copy link
Contributor

mcm001 commented Apr 14, 2025

Tempted to merge this as a not-squash to preserve history and make bisecting easier

@samfreund
Copy link
Member

Tempted to merge this as a not-squash to preserve history and make bisecting easier

Sure. Imma put another blocking review so that we remember.

Copy link
Member

@samfreund samfreund left a comment

Choose a reason for hiding this comment

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

Rebase when we merge

Gold856 added 6 commits April 14, 2025 18:43
This allowed us to drop a few Apache Commons dependencies, which is good for reducing JAR size and the number of things we need to pull in.
printTestResults was duplicated a lot, so it has been moved to TestUtils for maintainability.
QuirkyCamera was refactored to use EnumMap, since that seems to be more efficient. This also enables more concise copying of QuirkyCamera in getQuirkyCamera by just passing in the Map of the QuirkyCamera. Also, putAll was used instead of manually copying quirks in updateQuirks for conciseness.
@Gold856
Copy link
Contributor Author

Gold856 commented Apr 15, 2025

Cleaned up history for a rebase merge so some of the commits have nice summaries and descriptions.

@Gold856 Gold856 linked an issue Apr 16, 2025 that may be closed by this pull request
@mcm001 mcm001 requested a review from samfreund April 20, 2025 00:16
@mcm001 mcm001 enabled auto-merge (rebase) April 20, 2025 00:16
@mcm001
Copy link
Contributor

mcm001 commented Apr 20, 2025

FUCK IT EINSTEIN IS OVER WE BALL LETS GET THIS BREAD

@mcm001 mcm001 disabled auto-merge April 20, 2025 00:17
@mcm001 mcm001 merged commit ad1f51b into PhotonVision:main Apr 20, 2025
39 checks passed
@Gold856 Gold856 deleted the refactor branch April 20, 2025 00:35
@Gold856 Gold856 mentioned this pull request Apr 20, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle kUYVY and kY16 pixel formats
3 participants