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

Player lookat #58

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

osadchiyilya
Copy link

Several things exposed to lua scripts to allow improvements to player lookAt.

  • Get introduction state and nickname between chars
  • Get and set character "short description" (previously existed in DB but was not loaded into player class).

@osadchiyilya
Copy link
Author

@vilarion or @mkaring, please take a look at this when you have time.

@mkaring
Copy link
Member

mkaring commented Feb 14, 2021

The only thing of concern that comes to mind, is the missing check for the length of the description. The database only accepts 255 characters. If a description exceeding this length is stored by the scripts, saving the character will fail during logout.

@osadchiyilya
Copy link
Author

Thanks! I'll add the length verification.

@estralis estralis requested a review from vilarion April 30, 2021 04:21
Copy link
Member

@vilarion vilarion left a comment

Choose a reason for hiding this comment

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

What do you think about having only one getDescription / setDescription with player language as parameter?
That way we could spare scripts some logic to determine the required language. Granted, on setDescription you would probably pass Player.german or Player.english, but it should be an improvement for get... when showing the description to some random player. Also it feels a bit strange to scale the number of functions with the number of languages.

@osadchiyilya osadchiyilya marked this pull request as draft July 10, 2021 16:50
@osadchiyilya
Copy link
Author

I agree that having language as parameter feels better. I will implement it (when my rl gets under control).
Just to explain why I chose to implement it like this in the first place:

  • This is the style all over the existing code
  • It seems that adding another language to illa would require fixing lots of code (I've seen many places with if-else on language that asdume only 2 options)
  • The logic of get is going to be somewhat complicated (if player is english-speaker but target has only german description I want to show it, for the sake of bilinguals and google-translaters). But maybe this complicated logic will still be simpler if language is a parameter.

@vilarion
Copy link
Member

* This is the style all over the existing code

Yes, but a lot of the server code does not provide a good example.

* The logic of get is going to be somewhat complicated (if player is english-speaker but target has only german description I want to show it, for the sake of bilinguals and google-translaters). But maybe this complicated logic will still be simpler if language is a parameter.

I think the complicated logic should be left to the scripts, while the server just returns the strings.

I can see your points, which is why I wasn't entirely sure myself. In the end it just felt weird to have it designed with multiple functions like that.

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