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

Units Page #279

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

CollinsSpencer
Copy link
Contributor

  • Parse unit definitions from the current game instance
  • Parse unit language from the current game instance based on locale*
  • Pull unit images from https://bar-rts.com/unitpics3d/${unitId}.png
  • Persist unit data in IndexedDB
  • Dynamically display unit information on the Library > Units page

List view:
image

Grid view:
image

* Hardcoded to 'en' for now

@p2004a
Copy link
Contributor

p2004a commented Nov 9, 2024

  • I don't like any runtime dependencies on random URLs not dedicated for the use case which https://bar-rts.com/unitpics3d is in my book at the moment. I also overall have objection about runtime dependency on https://bar-rts.com/ as it doesn't have any reliability hardening whatsoever.
  • I'm also not in favor of having plenty of dependency on game archive internal files, especially without any tests in the main game repo that are going to verify compatibility.

So, maybe it's fine working functionality, but those 2 points are just additional launch blockers for new lobby for players if this gets merged.

@bcdrme
Copy link
Contributor

bcdrme commented Nov 9, 2024

  • I don't like any runtime dependencies on random URLs not dedicated for the use case which https://bar-rts.com/unitpics3d is in my book at the moment. I also overall have objection about runtime dependency on https://bar-rts.com/ as it doesn't have any reliability hardening whatsoever.
  • I'm also not in favor of having plenty of dependency on game archive internal files, especially without any tests in the main game repo that are going to verify compatibility.

So, maybe it's fine working functionality, but those 2 points are just additional launch blockers for new lobby for players if this gets merged.

What do you suggest because we are in dire need of those services existing imo.
Do you want to create projects similar to map-metadata for these use-cases ?

  • units info,
  • replays,
  • patch notes,
    what else is out there ...

@p2004a
Copy link
Contributor

p2004a commented Nov 9, 2024

What do you suggest because we are in dire need of those services existing imo.
Do you want to create projects similar to map-metadata for these use-cases ?

  • units info,

Having units info in lobby is a nice to have. For creating well supported dumps of units information see beyond-all-reason/Beyond-All-Reason#3039 and https://discord.com/channels/549281623154229250/549282166543089674/1240060975751761920.

Parsing lua code like it's done here, and not evaluating it under engine, will never be able to properly resolve properties like https://github.com/beyond-all-reason/Beyond-All-Reason/blob/55e3c659e614c502a7de1dee03b15f14f9bb96a6/units/corassistdrone.lua#L36

Unitpics are just hardcoded in https://github.com/beyond-all-reason/bar-live-services/tree/master/static/unitpics, I don't know how does maintenance of them look like and if they are up to date with https://www.beyondallreason.info/units/armada-bots and is semi manually managed.

  • replays,

What about replays needs to be integrated in lobby? Replays hosting is a separate web service.

  • patch notes,

Game Devs don't do any patch notes at all currently, there don't exist patch notes for game repo.


Overall, this is great body of work, and it seems to be working, but... all the buts above... I really don't know what to do with this PR.

@CollinsSpencer
Copy link
Contributor Author

Product Concerns

  • I created this PR as a POC for getting a units page working in the game. It's my first contribution, so I wanted to work on something "nice to have" / not functionally required to orient myself with the repo.
  • The units page is the main part of the website that I utilize, so I thought this would be a worthwhile addition. The problem it solves for me is knowing information about the units without having to start a game battle.
  • As for UX/UI, I based it on the website, so I wouldn't be re-inventing everything. Yes, it definitely could be improved/expanded to have more filters, data, and usability, but the basics are there.

Technical Concerns

  • Data source options
    • Parse game lua files without runtime (currently how I'm getting unit metadata)
    • Parse game lua files with runtime
    • Package a json file with data in game internal files
    • Include directly in this repo as assets
    • CDN
      • This is the best option for a single source of truth between the website and the lobby.
  • Unit metadata
    • Distinct for each version of the game.
    • I was pulling this data from the game internal files so that it wouldn't need an additional network request to fetch the data, helping with (truly) offline gameplay.
    • Since a network is required to download a new version of the game, these values could easily be retrieved and cached at the same time.
  • Unit pictures
    • Do these change often? I'm assuming it would be safe to use a single source of truth for these pictures.
    • I agree that random URLs as runtime dependencies is not ideal. I used bar-rts.com since it appeared to be a production-ready source for the images (it's already in use on the website). It sounds like a proper CDN is needed though, and bar-rts should be updated to use that CDN as well.

@bcdrme
Copy link
Contributor

bcdrme commented Nov 12, 2024

Jump on Discord so we can coordinate our efforts @CollinsSpencer
We're in the process of designing the new lobby, and prioritizing tasks. You're very welcome to join us :)

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