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

Import all sprites in the project #10680

Merged
merged 6 commits into from
Feb 14, 2025
Merged

Import all sprites in the project #10680

merged 6 commits into from
Feb 14, 2025

Conversation

Tristramg
Copy link
Contributor

Ref #10627

What does this pull request do

This pull requests imports the sprites to have them locally instead of a remote server (4f6659c)

We also change the tool to generate. This reduces a bit the size of the generated atlas and is — arguably — a little bit easier to use 92a75ff

The front now uses those new sprites f60ec38

Benefits

  • Less dependency to an external server
  • Smaller files (~1Mb saved per map view)

How to test

There should be no behavior change.

Navigate through the map with the signal layer on. See in the console if there are no missing sprites and in the network tools that all sprites files are correctly loaded from the osrd server and no longer https://static.osm.osrd.fr/sprites/sprites

In editoast, run generate-atlas.sh (it shouldn’t change anything, but output what atlases have been generated). This requires spreet (cargo install spreet)

@Tristramg Tristramg requested review from a team as code owners February 4, 2025 23:07
@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Feb 4, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.92%. Comparing base (80a92a4) to head (8caf39b).
Report is 19 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10680      +/-   ##
==========================================
- Coverage   81.93%   81.92%   -0.01%     
==========================================
  Files        1083     1083              
  Lines      107554   107553       -1     
  Branches      741      741              
==========================================
- Hits        88124    88118       -6     
- Misses      19389    19394       +5     
  Partials       41       41              
Flag Coverage Δ
editoast 74.30% <100.00%> (-0.02%) ⬇️
front 89.46% <100.00%> (ø)
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 87.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Tristramg Tristramg self-assigned this Feb 4, 2025
@emersion
Copy link
Member

emersion commented Feb 4, 2025

How large are these new assets?

@Tristramg
Copy link
Contributor Author

When taking the largest on my computer (@2x, I suppose this would be the same for most users), that used to be hosted separately

  • Before : 1.28 Mb
  • After : 409 kb

You can see in the diff on github the size reduction for the existing sprites (~40%)

@Tristramg Tristramg requested a review from flomonster February 5, 2025 10:17
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

You are using way too much sprites.
For example all "Arret *" are not used at all (referenced only in SignalsName.ts).

It's annoying to put all sprites under signal_sprites when some are not signals (e.g. “HEURTOIR”).

  • We could rename this folder to `sprites.
  • We could move the default sprites in another folder.

@Tristramg
Copy link
Contributor Author

Thank you for the feedback

  1. I move signal_sprites to sprites (folder with the assets and its path in sprites.rs) : 671e696
  2. Instead of just dumping all the icons of the previous sprite, I looked in the code : 49bb8e2
  • grep icon-image
    * MATD
    * MATG
    * HEURTOIR
    * UNKNOWN2 (for holyland)
    * UNKNOWN
    * TIV D FIXE *
    * TIVD B FIX *
    * R
    * Z
    * RIGHT
  • select distinct(data->'type') from infra_layer_neutral_sign;
    • CC FIN
    • REV
    • BP DIS
    • CC EXE
    • BP EXE
    • SECT (this one is missing in the blueprint)
    • BP FIN
  • Actually the blue icons are used in the blueprint style for neutral_sign. Sorry for that, I added them
  • I just kept used images and their blue counterpart

As a result, the served PNG is now less than 300kb big, and of course we need less space in the repo

Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM, I wonder if we want to keep the blueprint style. It was there to visualize the infra schematic (which no longer exists).

We should ask @thibautsailly. If it's not essential to keep this style, we could drop all the blue sprites.

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested, sprites are still there :) (except the remaining error for etcs)

They are now built either during the docker build
Or must be generated by hand as described in the readme

Signed-off-by: Tristram Gräbener <[email protected]>
…rch in subdirectories

Signed-off-by: Tristram Gräbener <[email protected]>
We added signals that are no longer only for signals

Signed-off-by: Tristram Gräbener <[email protected]>
This reduces a dependency to an external server

The sprites are also smaller
0.41Mb instead of 1.2Mb for the default sprite @2x

Signed-off-by: Tristram Gräbener <[email protected]>
The source is https://github.com/Nakaner/railway-signals/

The licence is CC-0, so we are good

Signed-off-by: Tristram Gräbener <[email protected]>
@Tristramg Tristramg added this pull request to the merge queue Feb 14, 2025
Merged via the queue into dev with commit a9148c7 Feb 14, 2025
27 checks passed
@Tristramg Tristramg deleted the all_sprites branch February 14, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants