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

[Magiclysm] Technomancer Mapping Spell #72148

Closed
wants to merge 7 commits into from

Conversation

rty275
Copy link
Contributor

@rty275 rty275 commented Mar 3, 2024

Summary

Mods "Technomancer map spell"

Purpose of change

At present, the only magiclysm class with a mapping spell was Earthshaper. This opens the ability up to all characters.

Describe the solution

Added a spell that lets technomancers map areas, modeled after the earthshaper spell.

Describe alternatives you've considered

There are a couple of ways I wanted to be able to do this. Making it use a pseudo-map like the earthshaper spell that only revealed MAN_MADE structures would have been great, but I didn't see a means of doing that and it would have been a nightmare to try and add every single viable location to a map. I also had thought it might be cool to place the map radius around the nearest radio tower rather than around the player, but I didn't see a way of doing that either. I'm sure it's not theoretically impossible to do those things, but it would take more skill at coding than I have, so this is what I got.

Testing

Spell casts properly, learns properly, and gives proficiencies properly. Spell scroll spawned properly in the item groups it was added to.

Additional context

While testing this I discovered that while the Novice Channeling proficiency correctly reduces cost and time to cast, Apprentice and Master seem to increase it again? I don't think that's an error with this PR given how I'm using the same math as every other spell, but it's worth looking into. EDIT: Made a PR to fix it, ended up being much easier than I thought.

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Mods Issues related to mods or modding Spawn Creatures, items, vehicles, locations appearing on map Mods: Magiclysm Anything to do with the Magiclysm mod Mechanics: Enchantments / Spells Enchantments and spells labels Mar 3, 2024
@github-actions github-actions bot requested a review from KorGgenT March 3, 2024 18:54
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Mar 3, 2024
@Maleclypse
Copy link
Member

I don’t want to be a negative naysayer here but this is not reasonable. The only justification you’ve given is that people who choose technomancer over earthshaper don’t get map revealing spells. That’s a net good that they don’t. I’m fully against any class pairing ever having parity of utility. So the bar for this would already be high to me. But when you can’t make it thematically different so that there is at least some lore friendly reason for why they would get something similar I’m even more against it.

@Maleclypse
Copy link
Member

I just saw in #mod-dev where Korg said no to this.

@Maleclypse Maleclypse closed this Mar 3, 2024
@rty275
Copy link
Contributor Author

rty275 commented Mar 3, 2024

That was a no to the previous spell justification (satellite hookups), which I have since changed. I understand not wanting class parity though, but I do want to be clear that I didn't just make a PR based on something that was already said no to, my understanding was that Korg just said no to the theming, not the effect.

@Maleclypse
Copy link
Member

i think a technomancer spell that gives you better-than-binocs omt reveal benefit would be distinct enough. just copying the earthshaper spell, no And I believe Guardian just jsonized that so you could make a recipe to improve binocs.

@rty275
Copy link
Contributor Author

rty275 commented Mar 3, 2024

Oh really? I'll figure out how to do something like that then! Sorry about the confusion, I thought just changing the theming was enough but it does make sense that a plain map reveal is not really thematically appropriate. Still wish I could make it a radius around a radio tower or something instead, but that's definitely outside of my abilities.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 3, 2024
@kevingranade
Copy link
Member

As @Maleclypse says, "to give parity with other school" isn't a valid rationale for including something. The whole point of schools is so that magic is very different depending on which ones you have access to.

We were talking about a magic drone based spell in discord, I'm not totally clear whether just having it appear as an intangible part of the spell fits magiclysm or whether you'd want a concrete drone item, but I think it's a much better thematic fit.

@rty275 rty275 deleted the technomancer-map-spell branch March 8, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mechanics: Enchantments / Spells Enchantments and spells Mods: Magiclysm Anything to do with the Magiclysm mod Mods Issues related to mods or modding Spawn Creatures, items, vehicles, locations appearing on map
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants