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

Mutate base records when adding/removing spells #2954

Merged
merged 2 commits into from Jul 29, 2020

Conversation

Assumeru
Copy link
Contributor

@Assumeru Assumeru commented Jul 4, 2020

Split from #2870. This doesn't so much implement vanilla behaviour as vanilla-adjacent behaviour. It should get a good bit of testing.

Instances of the same actor in vanilla share a single in-memory spell list drawn from the base record. GetSpell appears to interact with this list with the interesting side effect that it cannot detect racial abilities. Adding spells to one actor adds them to all instances instantly. (Though visual effects don't get updated until the cell is reloaded.) Basically it's weird all around.

Calling AddSpell on an NPC adds the spell to the base record, provided the NPC doesn't have autocalculated stats. This PR adds it to the base record regardless. (Saving and reloading would also clear any added spells for autocalculated actors.)

The Cure Disease effect can conditionally remove spells (diseases) from the base record. Example:

[instance A]->AddSpell collywobbles
[instance A]->GetCommonDisease == 1
[instance B]->GetCommonDisease == 1
Cast "cure common disease other" on [instance B]
[instance A]->GetCommonDisease == 0
[instance B]->GetCommonDisease == 0
Save and reload
[instance A]->GetCommonDisease == 1
[instance B]->GetCommonDisease == 1
Cast "cure common disease other" on [instance A]
Save and reload
[instance A]->GetCommonDisease == 0
[instance B]->GetCommonDisease == 0

This PR makes both instances affect the base record equally.

apps/openmw/mwmechanics/spelllist.cpp Outdated Show resolved Hide resolved
apps/openmw/mwmechanics/spelllist.cpp Outdated Show resolved Hide resolved
apps/openmw/mwmechanics/spells.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@zinnschlag zinnschlag left a comment

Choose a reason for hiding this comment

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

Other than the one minor stylistic issue I commented on inline I can not see any problems with the implementation.

However the question is how close should we emulate vanilla behaviour? We have diverged from vanilla in a few situations in the past, when vanilla was completely broken. This comes pretty close to such a situation.

The real question is how much would we break if we went for something more reasonable (let's say only have actual spells affect all instances of the same record and let things like diseases work on a per actor basis). Maybe that should be discussed with the wider MW community.

Anyway, this required indeed a lot of testing, no matter what we do. I doubt that we can get enough testing purely from the devs. And since we are early in the current release cycle, it would make sense to merge soon to get the widest possible range of testing.

apps/openmw/mwmechanics/spells.hpp Outdated Show resolved Hide resolved
@Assumeru
Copy link
Contributor Author

Assumeru commented Jul 6, 2020

However the question is how close should we emulate vanilla behaviour?

Yeah, it honestly just felt like they were trying to optimise something here and that led to these quirks. On the plus side, I now understand why diseased creatures have scripts that reapply the disease. On that note, the ability to add diseases to all instances of a creature seems interesting for modding.

@zinnschlag
Copy link
Contributor

On that note, the ability to add diseases to all instances of a creature seems interesting for modding.

As a separate option, yes. But then, we could also make what we have now this option and add another function that only affects individual actors later. That is probably the safer bet.

apps/openmw/mwmechanics/spelllist.cpp Outdated Show resolved Hide resolved
apps/openmw/mwmechanics/spelllist.cpp Show resolved Hide resolved
apps/openmw/mwmechanics/spells.hpp Show resolved Hide resolved
apps/openmw/mwmechanics/spells.cpp Outdated Show resolved Hide resolved
@Assumeru Assumeru force-pushed the basespells branch 2 times, most recently from 18e65a8 to bd61e49 Compare July 12, 2020 10:09
@Assumeru
Copy link
Contributor Author

Anyone have any more change requests?

@psi29a
Copy link
Member

psi29a commented Jul 27, 2020

Merge conflicts ;)

@Assumeru
Copy link
Contributor Author

With myself even, my favourite. Rebased.

@psi29a
Copy link
Member

psi29a commented Jul 28, 2020

I see that you also modify the save game format. Are we at least backwards compatible? (old save games will work)

@Assumeru
Copy link
Contributor Author

Of course.

With spells not being stored in the base record older versions just won't be able to find them, hence the increment.

@psi29a psi29a merged commit 72b63ed into OpenMW:master Jul 29, 2020
@akortunov
Copy link
Collaborator

https://gitlab.com/OpenMW/openmw/-/issues/5566

This PR causes issues so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants