{"action":"created","issue":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/944","repository_url":"https://api.github.com/repos/Interkarma/daggerfall-unity","labels_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/944/labels{/name}","comments_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/944/comments","events_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/944/events","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/944","id":368473179,"node_id":"MDExOlB1bGxSZXF1ZXN0MjIxNjQzNDcz","number":944,"title":"Tally magic skills when player casts spells","user":{"login":"numidium","id":5565027,"node_id":"MDQ6VXNlcjU1NjUwMjc=","avatar_url":"https://avatars0.githubusercontent.com/u/5565027?v=4","gravatar_id":"","url":"https://api.github.com/users/numidium","html_url":"https://github.com/numidium","followers_url":"https://api.github.com/users/numidium/followers","following_url":"https://api.github.com/users/numidium/following{/other_user}","gists_url":"https://api.github.com/users/numidium/gists{/gist_id}","starred_url":"https://api.github.com/users/numidium/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/numidium/subscriptions","organizations_url":"https://api.github.com/users/numidium/orgs","repos_url":"https://api.github.com/users/numidium/repos","events_url":"https://api.github.com/users/numidium/events{/privacy}","received_events_url":"https://api.github.com/users/numidium/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-10-10T02:50:35Z","updated_at":"2018-10-10T04:19:36Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/pulls/944","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/944","diff_url":"https://github.com/Interkarma/daggerfall-unity/pull/944.diff","patch_url":"https://github.com/Interkarma/daggerfall-unity/pull/944.patch"},"body":"This is my first time touching the effects system. If I'm doing something wrong then feel free to let me know."},"comment":{"url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/comments/428432651","html_url":"https://github.com/Interkarma/daggerfall-unity/pull/944#issuecomment-428432651","issue_url":"https://api.github.com/repos/Interkarma/daggerfall-unity/issues/944","id":428432651,"node_id":"MDEyOklzc3VlQ29tbWVudDQyODQzMjY1MQ==","user":{"login":"Interkarma","id":10426244,"node_id":"MDQ6VXNlcjEwNDI2MjQ0","avatar_url":"https://avatars3.githubusercontent.com/u/10426244?v=4","gravatar_id":"","url":"https://api.github.com/users/Interkarma","html_url":"https://github.com/Interkarma","followers_url":"https://api.github.com/users/Interkarma/followers","following_url":"https://api.github.com/users/Interkarma/following{/other_user}","gists_url":"https://api.github.com/users/Interkarma/gists{/gist_id}","starred_url":"https://api.github.com/users/Interkarma/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Interkarma/subscriptions","organizations_url":"https://api.github.com/users/Interkarma/orgs","repos_url":"https://api.github.com/users/Interkarma/repos","events_url":"https://api.github.com/users/Interkarma/events{/privacy}","received_events_url":"https://api.github.com/users/Interkarma/received_events","type":"User","site_admin":false},"created_at":"2018-10-10T04:19:36Z","updated_at":"2018-10-10T04:19:36Z","author_association":"OWNER","body":"Thank you for making a start on this. :)\r\n\r\nI'd recommend not performing skill tally in effect base. This would cause player to get a free skill increase every time they load a game, as effect is resumed on load. As a general rule, effects shouldn't make direct changes to entity state.\r\n\r\nThere's a few other little gotchas as well.\r\n\r\nEffects should only get a skill tally when they target an entity, including self. Casts that miss a target (e.g. shooting fireballs into air or at walls) shouldn't be tallied. EntityEffectManager.AssignBundle() is a better spot. Try the following around line 342 before absorb check.\r\n\r\n    // Tally magic skills\r\n    if (sourceBundle.CasterEntityBehaviour == GameManager.Instance.PlayerEntityBehaviour)\r\n    {\r\n        GameManager.Instance.PlayerEntity.TallySkill((DFCareer.Skills)effect.Properties.MagicSkill, 1);\r\n    }\r\n\r\nWe also need to ensure skill tally for effect isn't counted multiple times when AoE hits several targets at once. I think best solution here is to add a bool in EntityEffectBundle at line 69.\r\n\r\n    /// <summary>\r\n    /// Gets or sets if skills have been tallied for this bundle.\r\n    /// </summary>\r\n    public bool SkillsTallied { get; set; }\r\n\r\nThen back in EntityEffectManager.AssignBundle() line 342, change code to the following:\r\n\r\n    // Tally magic skills\r\n    if (sourceBundle.CasterEntityBehaviour == GameManager.Instance.PlayerEntityBehaviour && !sourceBundle.SkillsTallied)\r\n    {\r\n        GameManager.Instance.PlayerEntity.TallySkill((DFCareer.Skills)effect.Properties.MagicSkill, 1);\r\n        sourceBundle.SkillsTallied = true;\r\n    }\r\n\r\nAs the same bundle payload instance is handed to each target entity for instantiation, the flag will ensure skills are only tallied once for that instance.\r\n\r\nI don't think we need to worry about other effects being counted (e.g. diseases, poisons) as they shouldn't have a caster set. But probably can't hurt to exclude non-spell bundles in the above check just in case. So something like the below at line 342 ought to have us covered.\r\n\r\n    // Tally magic skills\r\n    if (sourceBundle.CasterEntityBehaviour == GameManager.Instance.PlayerEntityBehaviour && !sourceBundle.SkillsTallied && sourceBundle.Settings.BundleType == BundleTypes.Spell)\r\n    {\r\n        GameManager.Instance.PlayerEntity.TallySkill((DFCareer.Skills)effect.Properties.MagicSkill, 1);\r\n        sourceBundle.SkillsTallied = true;\r\n    }\r\n\r\nI hope that helps! Let me know if anything I can do to assist. I'd also be interested in feedback from @Allofich around magic skill tallies specific to classic."}}
{"actor":{"display_login":"Interkarma"}}