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

Arcana: fixes/rebalancing from archived repository #19247

Closed

Conversation

Projects
None yet
4 participants
@DangerNoodle
Copy link
Contributor

commented Nov 11, 2016

This implements no new content, presently only balancing and other alterations from the repository being maintained by @chaosvolt.

The main change appears to be changing the martial art overrides to avoid any future changes to the affect arts causing issues, by implementing use of copy-from.

Additionally, several mapgen entries retained the defunct use of 'place_items' instead of the current use of 'place_loot'. That has now been corrected.

There was also one apparent typo fix along with a few alterations to professions, recipes, and tools.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2016

Good. I'll look at what you ported over and explain my reasons for why I added the

  • Martial art fix: You've already explained this one, and you wrote it up to begin with. Futureproofing.
  • Typo fix: Scrolls of darkness were originally called scrolls of mysticism, and one instance of the old name was retained. Derp. :V
  • Profession changes: Balancing. Mask of insight makes hammer useful for its actual effect, scribe and summoner are ill-fitted for a dangerous scenario, while Dark Priest was intended to start with a symbol of judgement.
  • Recipe changes: Lore. The Six Pillars and History of Alchemy are present in the Cleansing Flame's book list, so letting them make the relevant items with dull essence is logical.
@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2016

Understood. As I am not implementing your addition of basement mapgen entries, I will hope that these additions are deemed acceptable.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2016

In the meantime, I'm mucking about with a map item to add to my repo eventually. Planning to have it map out the arcane structures, strange temples, and strange cabins.

Dunno where to make it spawn, but probably high odds in arcane structures so players that conquer one structure have an edge on finding others.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2016

Discovery, arcane ranged weapons are only reloading 1 at a time. Example discovered, symbol of judgement, explicitly does not have the required flag to force that behavior.

Edit, and they seem to have volume now? I thought lack of volume was explicitly intended.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2016

Oh. It would appear that reloading of non-magazine weapons is broken.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2016

...eh. How bad is it, a mL per essence? Mess with it if you want, but shouldn't really hurt anything. If you DO zero out volume, might have to make it a big more clear via a comment that the volume is intentional, not a redundant field. Unless 0 now defaults to 1 mL, which would be weird but not a big deal.

Weapons being broken in general is a more pressing concern. I fear that it, like the stair issue, will go unfixed for a good while.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2016

Also to fix: curious structure uses old "place_items" instead of place_loot. Island temples also borked the same.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2016

Updating my repo now. If you do copy these fixes for this, double-check and remove references to 'arcanemap' since that is a new item, and this is for fixed stuff only. :V

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2016

Thank you. I will have to add this when I am able to in the morning.

Noodle
@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2016

Apologies for the delays. Implementing fixed mapgen entries now. Note that you have an excess separator in 'mapgen_sanguine.json', the 'place_loot' entry where you added 'arcanemap'. Since I had to remove this loot spawn, the separator error is thus fixed on my end.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2016

Doh. Got it, fixed on my end now.

@mugling

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2016

copy-from support for many of these JSON types was lost when the PR implementing them was later reverted. Any further changes to this mod are best made in the relevant third-party repo.

@mugling mugling closed this Nov 25, 2016

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2016

Then what method is used instead of copy-from?

Closing this does not provide anything productive, I still would want to fix up the main version of the mod.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2016

@mugling, what is your problem?

I didn't receive any info about copy-from being broken until just now. Continuing to prove my point about this "too lazy to check mods" kick of yours is just making shit worse for everyone.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2016

Tell me what fixes are broken, and I can replace them with functional fixes and re-open, allowing Chaosvolt to fix it in their repository as well.

Closing the pull request because of feature breakage is not the ideal solution.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

Requesting @Rivet-the-Zombie to confirm whether Mugling's claimed reason for closing this is valid.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

I just tested my repo of Arcana with the latest available build, 5946. No load errors, and martial arts replacements still work as expected, and monsters are still edited to drop essence/items. As those are the only uses of copy-from in the mod, Mugling is either lying or acting in ignorance. Either way, he is not justified in closing this PR.

@DangerNoodle, re-open this.

@mugling, don't start closing stuff unless you have a valid reason.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

@chaosvolt I am unable to re-open.

@mugling, fixing and futureproofing are valid reasons for a pull request. No new content is being added in this pull request. The martial art changes in particular are geared towards ensuring that further changes (like with the removal of diamond weapon IDs) do not require anything else to be updated. Why would you object to a change intended to reduce the amount of effort being required in the future, especially as development does not have the amount of people needed to deal with the issues that already exist? This is illogical.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

For as long as we support a mod, we should allow fixes like that.

We don't have the full mod support up yet (ie. stable isn't out), so we should keep it up at least until that stable. We can do giant cleaning after the stable is out. Or as one of the last PRs before stable.

Keeping a broken mod in repo is the worst option possible.

@chaosvolt

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

Thank you.

@mugling

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2016

The main change appears to be changing the martial art overrides to avoid any future changes to the affect arts causing issues, by implementing use of copy-from.

This doesn't actually do anything as copy-from isn't yet supported for martial arts. Support was reverted out in #19137.

For as long as we support a mod, we should allow fixes like that.

This mod is horrible to support. The previous maintenance PR ended with a rebuke from a different developer. I'm going to mark this mod as obsolete which means the mod will still load but new games will need to be started using the version distributed from the third-party repo. Significant bugs can be PR'd to the core repo until it is deprecated but any extension needs to take place in the third-party repo.

@CleverRaven CleverRaven locked and limited conversation to collaborators Nov 26, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.