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

standardized spells and cantrips for armor pieces #1864

Merged
5 commits merged into from
May 8, 2019

Conversation

dgarson
Copy link
Contributor

@dgarson dgarson commented May 7, 2019

There are currently many missing cantrip spells from armor pieces. For example, only head/gaunts can come with cantrips for war magic, or have focus on them. All of the armor pieces should be eligible for all of the spells that can come on any armor coverage, rather than select spells being candidates for only some armor coverage.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

If all of the armor pieces spell arrays have all the same spells, there isn't any point in having separate arrays with lots of code duplication. One of the goals of revamping the loot gen system, and coding in ACE in general, is to reduce code duplication.

@dgarson
Copy link
Contributor Author

dgarson commented May 7, 2019

Seems reasonable. Shall I introduce another static array and reuse it for all the coverage slots?

@ghost
Copy link

ghost commented May 7, 2019

If each armor piece has the same potential spells and/or cantrips, then those separate arrays should be compressed down to fewer. Jewelry obviously needs to have its own, as those items don't have impen/banes.

@ghost
Copy link

ghost commented May 7, 2019

On the right track. ClothingCantrips looks to me to be much the same, also. I imagine that the normal spells are also in similar need of compression and standardization. Lastly, if those spell arrays are being condensed down to single arrays, the better option would be to remove the switch statements in LootGenerationFactory_Clothing.cs in favor of spells = LootTables.ArmorSpells and cantrips = LootTables.ArmorCantrips, rather than assigning the actual array to the others that are now unneeded.

@dgarson
Copy link
Contributor Author

dgarson commented May 7, 2019

Thanks for the feedback, Theran! Let me know if there's any more changes you'd recommend.

@dgarson dgarson changed the title standardized cantrip spells for armor pieces standardized spells and cantrips for armor pieces May 7, 2019
@ghost
Copy link

ghost commented May 7, 2019

Looks good

@dgarson dgarson closed this May 7, 2019
@dgarson dgarson reopened this May 7, 2019
@dgarson
Copy link
Contributor Author

dgarson commented May 7, 2019

Woops, didn't mean to close without merging. Didn't realize I have to wait for you to merge :-)

@ghost ghost merged commit 805e388 into ACEmulator:master May 8, 2019
This pull request was closed.
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.

None yet

1 participant