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

Added translations #153

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

Conversation

asodugf12
Copy link

Translations. Checked against minecraft translation files

@asodugf12 asodugf12 marked this pull request as draft October 19, 2024 14:49
@asodugf12 asodugf12 marked this pull request as ready for review October 19, 2024 14:50
pumpkin/src/server/translation.rs Outdated Show resolved Hide resolved
pumpkin/src/server/translation.rs Outdated Show resolved Hide resolved
@Snowiiii
Copy link
Member

I would like to use Config values are introduced like in the image in #120. You can seperate PR's so its easier for you to implemented, I would first begin to just use the client-side translations. You then can move on to server-side, I would also consider a better approche then reading hashmap's everytime, Im considering caching translations depending if they are loaded by players

@asodugf12
Copy link
Author

Added all the suggestions :>

@asodugf12
Copy link
Author

I think caching could be left to a config value that would be turned on automatically if the server has used it in the last 5 times it started. What would you recommend for caching?

@asodugf12
Copy link
Author

Finally fixed cargo fmt file diff error haizzz
Is this way of setting up TranslationsConfig okay?

@Snowiiii
Copy link
Member

this is still not the solution i wanted, Im sorry but implementing the language system i want, im pretty sure, is more complex than you think. I would like to do it myself. We have to put all languages from Minecraft into the server and copy then over. Not only provide one single path

@asodugf12
Copy link
Author

I am still unsure what solution you are thinking of, but I'm okay with you finishing the rest. I'll find some more issues to work on in the meantime. Thanks for guiding me, Alex :>

@asodugf12
Copy link
Author

Actually you know what, I still want to see this through the end. So if it's possible to make a detailed diagram of what functionality is needed and goes where, I'd love to work on this further

@Snowiiii
Copy link
Member

My Plan.

  1. Check if translations are on in the Config (default yes). If not use en_us as default (copy that over)
  2. If Translations are on, Check if client-side translation are on in the Config (default yes). If client-side translations are on, we can just simply use TextComponent::translatable, If off, we will use translations for the server (copy them over).

I would prefer if you first add the ability to use client-side translations if they are on, otherwise or if translations are off just use default en_us, Im still not 100% about copying every Minecraft translation into Pumpkin. But you can start with this

@asodugf12
Copy link
Author

Some questions that I am having right now:

  1. Does the translate() function accept a &str or a TextComponent?
  2. Is the input by default en_us?
  3. Do we have to parse the TextComponent::translatable (aka fill in the %s with the data in the with field)
  4. If not, do we have to generate the with field of TextComponent::translatable?

@Snowiiii
Copy link
Member

Some questions that I am having right now:

1. Does the `translate()` function accept a `&str` or a `TextComponent`?

2. Is the input by default en_us?

3. Do we have to parse the `TextComponent::translatable` (aka fill in the `%s` with the data in the `with` field)

4. If not, do we have to generate the `with` field of `TextComponent::translatable`?
  1. It accepts a &str (e.g. "commands.gamemode.something"...), And returns a TextComponent.
  2. Yes, en_us is the default
    3 & 4: You will it using the with field (which we have)
    image

@asodugf12
Copy link
Author

I think the main translation logic is complete?
Working on file handling next

@asodugf12
Copy link
Author

added en_us lang files up until 1.17, will finish the rest later

@asodugf12
Copy link
Author

Okay, I have discovered that for versions from 1.12.2 back, minecraft uses a different format for en_us files, which would require a different parser. I'll work on it maybe tomorrow :D

@Snowiiii
Copy link
Member

Okay, I have discovered that for versions from 1.12.2 back, minecraft uses a different format for en_us files, which would require a different parser. I'll work on it maybe tomorrow :D

We only support the latest Minecraft version

@asodugf12
Copy link
Author

Oh, so that means this PR is complete?

@Snowiiii
Copy link
Member

Oh, so that means this PR is complete?

Please remove all those langauge files from all other versions than 1.21.3. We only need one translation file for the latest Minecraft version

@asodugf12
Copy link
Author

Alright, please list anything else that needs to be finished in this PR, because I feel like it's been lingering for too long now :D

@asodugf12
Copy link
Author

Finished with the suggestions :D

@asodugf12
Copy link
Author

Hello, any comment on the fixes? @Snowiiii

@Snowiiii
Copy link
Member

Snowiiii commented Dec 6, 2024

You should use serde_json to read the JSON translation file. You can fully remove the manuel reading and parsing from the file. And also rename the static variable from PATH to something better like EU_US.

@asodugf12
Copy link
Author

I am using serde_json, the current implementation just reads it line by line and turn each one into a json object, thus avoids having to read the whole file into memory, since it is one big json object.

@asodugf12
Copy link
Author

Or actually, do we cache read it once and cache it into memory? If so do you have any suggestions?

@asodugf12
Copy link
Author

Hallo, any more suggestions? I forgot to check the PR due to exams :D

@Snowiiii
Copy link
Member

I am using serde_json, the current implementation just reads it line by line and turn each one into a json object, thus avoids having to read the whole file into memory, since it is one big json object.

I'm fine to have it stored as a HashMap, I don't think that the file will be that large

@asodugf12
Copy link
Author

I see, do I also add a cache for the hashmap?

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.

2 participants