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

Add Categories into track files #68

Merged
merged 3 commits into from
Nov 4, 2020

Conversation

pehala
Copy link
Collaborator

@pehala pehala commented Oct 17, 2020

  • Track files now contain also all categories they reside in.
    • Deprecate and conserve TrackFileParser as it can only intentionally handle V1 format.
    • Correct TrackParser to use now is VersionedTrackFileParser
  • Restructure parsers
  • Convert existing tracks
    • Reduced from 3700 to 2055
  • Add converter CLI option for server
    • java -jar server.jar convert tracks - Converts all tracks in place, doesn't override already converted ones.

TODO:

  • Documentation
  • Fix tests

Closes #64.

@pehala pehala requested a review from PhilippvK October 17, 2020 14:01
@pehala
Copy link
Collaborator Author

pehala commented Oct 17, 2020

I might do a little polishing but it is definitely review ready

@pehala pehala changed the title Add Categories into track files WIP: Add Categories into track files Oct 17, 2020
@PhilippvK
Copy link
Owner

@pehala The Server Check failed in the GitHub CI Run.

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

Yep, based on my investigation it is caused by mocking and testing CLI with subcommands. It is a failure in tests not in the funcionality (at least from what I tested :D)

@PhilippvK
Copy link
Owner

Dump question, but are the converted track still in the repo? Can't find them at the moment.

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

They are supposed to be but seems like I forgot to add them. Good catch, Thanks!

@PhilippvK
Copy link
Owner

@pehala How to compile the server.jar with Maven with the failing tests?

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

mvn install -DskipTests

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

Tracks added now

@PhilippvK
Copy link
Owner

PhilippvK commented Oct 19, 2020

Okay. Here is my first impression:

  1. GREAT WORK
  2. The Championships are intentionally broken, right?
  3. Was there ever a Skip track button in the solo-mode?
  4. When testing the multiplayer the clients crashed when both players pressed Skip:
2020-10-19 11:23:51.105 java[59218:7494793] -[AWTView javaRole]: unrecognized selector sent to instance 0x7ff57b5ea500
2020-10-19 11:24:03.385 java[59218:7494793] -[AWTView javaRole]: unrecognized selector sent to instance 0x7ff57b5ea500
2020-10-19 11:24:09.004 java[59218:7494793] -[AWTView javaRole]: unrecognized selector sent to instance 0x7ff57b5ea500
java.lang.NullPointerException
	at java.util.Hashtable.put(Hashtable.java:460)
	at agolf.TrackCollection.addTrack(TrackCollection.java:12)
	at agolf.game.GamePanel.method333(GamePanel.java:288)
	at agolf.Conn.handlePacket(Conn.java:184)
	at agolf.Conn.dataReceived(Conn.java:24)
	at com.aapeli.connection.GamePacketQueue.run(GamePacketQueue.java:32)
	at java.lang.Thread.run(Thread.java:748)

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

  • Thanks! Although as you can see there a lot to be done in terms of bugs
  • Yes, the championships are broken intentionally, I think they were broken before (based on the code that returned No tracksets on the server :D) but if they werent I need to fix them.
  • I don´t know? If it is there, I certainly didn´t add it on purpose.
  • Thanks! I will look at it.

- Rewrite tests
- Restructure parsers
- Convert existing tracks
- Add converter application for server
@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

  • Bug with skip fixed
  • Also the skip button on singleplayer appears when you are over or close to average par.

@PhilippvK
Copy link
Owner

Impressive contributions!

@PhilippvK
Copy link
Owner

@pehala ready for merge? How about Documentation updates? Does the README already contain information on the Tracks migration?

@pehala
Copy link
Collaborator Author

pehala commented Oct 19, 2020

Nope, I still need to fix the testa and add the documentation. I will add the checkboxes into the description.

Btw: What would you like to see in the docs? I could write there the whole structure of the track file and how it is processed but that would be in the separate file

@PhilippvK
Copy link
Owner

@pehala Regarding the docs: I only thought about the essentials on how to get a server running. But I just realized that as all the files are already converted the exact process of migration is not really needed.

Details in the File structure are definitely optional, they also don't have to be in markdown format if you prefer to define them as comments inside the implementation only.

@pehala
Copy link
Collaborator Author

pehala commented Oct 31, 2020

I created issue for the mockito/picocli bug remkop/picocli#1243

@pehala pehala changed the title WIP: Add Categories into track files Add Categories into track files Nov 4, 2020
@PhilippvK
Copy link
Owner

Many thanks!

@pehala
Copy link
Collaborator Author

pehala commented Nov 4, 2020

Tests and documentation are now done.

@PhilippvK PhilippvK merged commit c03ce5d into PhilippvK:master Nov 4, 2020
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.

Add Category line into track file format
2 participants