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

Racial Skill and Rep Skill both show up #17

Closed
wduda opened this issue Dec 17, 2020 · 9 comments · Fixed by #32 or #29
Closed

Racial Skill and Rep Skill both show up #17

wduda opened this issue Dec 17, 2020 · 9 comments · Fixed by #32 or #29
Assignees
Labels
Milestone

Comments

@wduda
Copy link
Owner

wduda commented Dec 17, 2020

Looks like I'm not the only one with Thorin's Gate issues, but mine's the other way around - on my Dwarf Champion I see both Return to Thorin's Gate (racial) and Return to Thorin's Gate (rep). Obviously I can't learn the reputation one so it's greyed out, but strangely enough I can't remove it from the travel window.

(edit: I just realised something that might be important: this only started happening when I completed the deed to get the racial trait and equipped it. Before then, neither version would show)

It's not a game breaker, but it's quite bothersome nonetheless so I'm hoping you can locate the bug and find it's something that's easily fixed. Please?

Here's what I've tried:

  • in tab 'Select': untick Return to Thorin's Gate. This removed both from the window, so not a solution
  • in tab 'Sort': Return to Thorin's Gate shows up twice in the list, so I move the second one to the bottom. This did nothing, and when I went back to the Sort tab I saw that the skill had snapped back to its old position
  • open the data file and manually set the rep version of the skill to 'false', while keeping the racial version 'true', and then launching the game. This did nothing, but file was not altered back to 'true' after closing the game
  • delete the data file and launch the game to build the file back up from scratch. No change whatsoever

Here's what it looks like in-game:

@ejsmit
Copy link

ejsmit commented Dec 19, 2020

This was never a problem up to 28.1. There was no option to remove racial skills - they just always appeared when they were available. The one bit of config I always had to do was to untick the rep skill for my own race (man or dwarf only)

28.2 broke it by removing the (rep) from the rep skill names

repLocations:AddData("Return to Bree (Rep)", "0x7001BF90", "Return to Bree");

to

repLocations:AddData("Return to Bree", "0x7001BF90", "Return to Bree");

I seems that the addon does not like two list entries with the same name Return to Bree, the first for the rep and the second for the racial skill.

Renaming it as a test to

repLocations:AddData("Return to Bree-a", "0x7001BF90", "Return to Bree");

caused the following

  • unchecking the new rep skill Return to Bree-a causes only it to disappear and not the racial skill.
  • unchecking racial skill still does nothing - the racial skill always appears when available and no way to turn it off.

@wduda wduda added the bug label Dec 23, 2020
@wduda wduda self-assigned this Dec 23, 2020
@wduda
Copy link
Owner Author

wduda commented Dec 23, 2020

@ejsmit thanks for the additional info, that makes it easier to understand what is happening.

In the data stored in the plugin like repLocations:AddData("Return to Bree-a", "0x7001BF90", "Return to Bree"); the data is as follows:

  • the first string is the name of the skill in the game and unfortunately, the identifier used long ago by Dhor to identify skills, for reasons based in a limitation of the LUA API
  • the second string is the skill ID ingame that is unique and consistent across game client localisations, not well supported by the API though
  • the third string is just text used to label the skill in the plugin, something I intend to start using more for multiple features

As the plugin is using a non-unique identifier, racial skill and the reputation skill with the same name get mixed up. By adding the -a you have made it unique so it shows up, but if what this also causes is the shortcut becoming nonfunctional, so just renaming skills, unfortunately, does not solve our problem.

I am working on a way so these skills are identified differently, as this would allow a more robust way of identifying skills, remembering which ones are enabled and what the sort order is.

@wduda wduda changed the title Racial Skills show up two times Racial Skill and Rep Skill both show up Dec 23, 2020
@wduda
Copy link
Owner Author

wduda commented Jan 4, 2021

2- Under Travel Options->Select, if I un-check either "Rivendell (Racial)" or "Rivendell (Rep)" the skill display window removes both

@ejsmit
Copy link

ejsmit commented Jan 4, 2021

Rivendell is a store skill like Michel Delving. Other races cannot get it from rep.

https://lotro-wiki.com/index.php/Return_to_Rivendell

@wduda
Copy link
Owner Author

wduda commented Jan 4, 2021

Rivendell is a store skill like Michel Delving. Other races cannot get it from rep.

https://lotro-wiki.com/index.php/Return_to_Rivendell

Well, this is a simple change in the label to say "Store" rather than "Rep", and I can implement that easily. It will however not change any issues with sorting or the main issue here, unfortunately.

@ejsmit
Copy link

ejsmit commented Jan 4, 2021

Fixing the enabled value for skills seem to be relatively easy: just index settings.enabled by Data instead of by Key and it will work. I can now enable and disable skills on demand without any visible sideeffects.

The API is unfortunately very limited. As soon as you get either rep or racial, both are added to the travel window because they are searched by name. No easy way to fix that.

I can submit a PR if you are interested.

@wduda
Copy link
Owner Author

wduda commented Jan 4, 2021

PRs are always welcome, I would welcome it.

Using the ID as a unique identifier is the obvious choice; I am aware of that. I have not done that yet since what is labor intense or hard is not rewriting the code, but testing in the client and checking all the possible edge cases.

@wduda
Copy link
Owner Author

wduda commented Jan 5, 2021

I have committed code that first accomplishes the ability to enable/disable skills in options and all of them being referred by skillD. Meaning racial and reputation skill can be enabled/disabled separately. I still need to check the case where a character has their racial skill acquired but not the shop skill, and the untrained shop skill would show up as described in the original bug report. Once a release happens hopefully tomorrow this should be a significant improvement though.

@wduda wduda added this to the v1.0.7 milestone Jan 5, 2021
@wduda
Copy link
Owner Author

wduda commented Jan 5, 2021

Because of the way the API returns learned skills, it seems there is no way of knowing which of the two skills with identical names is learned and which not. Making it so they will both show up, and the user can decide to hide the one their character does not know. This at least is possible with the current code in develop.

@wduda wduda mentioned this issue Jan 7, 2021
This was linked to pull requests Jan 7, 2021
@wduda wduda closed this as completed Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants