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

Add recipe to make technomancer's toolbelt, add gold infuser and corresponding recipe, edited copper and silver infuser and their recipe #40011

Closed
wants to merge 32 commits into from

Conversation

martinrhan
Copy link
Contributor

@martinrhan martinrhan commented Apr 29, 2020

Summary

SUMMARY: Content "Add recipe to make technomancer's toolbelt, add gold infuser and corresponding recipe, edited copper and silver infuser and their recipe."

Purpose of change

technomacer's toolbelt should be craftable, and the infusers should be mainly requiring spellcraft instead of fabrication. Besides, there are copper infuser, silver infuser, so why don't add a gold one?

Testing

I edited the JSON files and runed the game to test them.

Additional Content

Also add a line of text document MAGIC.md for issue #39952

@martinrhan
Copy link
Contributor Author

Just realised that there is actually no change in this pull request.
Then opened the Github Desktop, however the button "commit to master" is not able to be clicked while there are 4 changed files displayed.

@mlangsdorf mlangsdorf changed the title Some edits about Magiclysm items and recipe Add recipe to make technomancer's toolbelt, add gold infuser and corresponding recipe, edited copper and silver infuser and their recipe Apr 30, 2020
@mlangsdorf mlangsdorf added the Mods: Magiclysm Anything to do with the Magiclysm mod label Apr 30, 2020
@martinrhan
Copy link
Contributor Author

I just commited my change, the reason why I was not able to commit it was I didn't write a summary

@KorGgenT
Copy link
Member

KorGgenT commented Apr 30, 2020

As they are, the recipes for the gold infuser bracelet and the technomancer tool belts are not expensive enough. You're just turning a survivor's toolbelt into a significantly better toolbelt just because you're a technomancer. similarly for the gold bracelet into an infusion bracelet.

i will say that i'm going to hold off on this PR until nested containers lands because then the toolbelt itself can get adjusted appropriately, but just buffing all of its tool qualities doesn't seem right.

@martinrhan
Copy link
Contributor Author

martinrhan commented May 1, 2020

As they are, the recipes for the gold infuser bracelet and the technomancer tool belts are not expensive enough. You're just turning a survivor's toolbelt into a significantly better toolbelt just because you're a technomancer. similarly for the gold bracelet into an infusion bracelet.

i will say that i'm going to hold off on this PR until nested containers lands because then the toolbelt itself can get adjusted appropriately, but just buffing all of its tool qualities doesn't seem right.

OK, I just made the belt a lot more difficult to make, and also reduced its abiility. But for the gold infuser, I am just repeating the pattern of copper infuser and silver infuser, I don't think it has any problem.

@martinrhan
Copy link
Contributor Author

and I also added a line of text to document MAGIC.md since it is just a small change I put it in this pull request instead of making another one

@martinrhan
Copy link
Contributor Author

can any one tell me what is "continuous-integration/travis-ci/pr" check? How to pass this check?

@anothersimulacrum
Copy link
Member

It's a set of several tests that are run to ensure that the game still builds on several problems, and some guidelines are followed. As you can see here, it is complaining that this PR does not follow a formatting guideline.

@@ -44,14 +44,27 @@
"result": "silver_infuser",
"category": "CC_ENCHANTED",
"subcategory": "CSC_ENCHANTED_TOOLS",
"skill_used": "fabrication",
"skills_required": [ "spellcraft", 3 ],
"skill_used": "spellcraft",
Copy link
Member

Choose a reason for hiding this comment

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

no. put this back. making infusers should not exercise your spellcraft. same for the gold one.

"time": "40 m",
"book_learn": [ [ "alchemy_basic", 3 ], [ "necro_basic", 4 ], [ "techno_basic", 4 ] ],
"qualities": [ { "id": "CHISEL", "level": 3 } ],
"components": [ [ [ "gold_bracelet", 1 ] ] ]
Copy link
Member

Choose a reason for hiding this comment

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

now that we are getting to sufficiently high levels of mana infusing, i want this to take something in addition to the bracelet. maybe some of the crystallized mana dust that was added fairly recently. 40 minutes also seems a little low for a supposedly complex recipe that you only need to craft once.

doc/MAGIC.md Outdated
@@ -60,6 +60,8 @@ For example, this is the formula for spell failure chance:

```( ( ( ( spell_level - spell_difficulty ) * 2 + intelligence + spellcraft_skill ) - 30 ) / 30 ) ^ 2```

(Just to make it clear, this is not the resultant value of failure chance, for more detail, see function spell_fail in magic.cpp.)
Copy link
Member

Choose a reason for hiding this comment

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

this is misleading. instead it should mention that it caps out at an effective spell level of 30 at 0% failure chance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it not really necessary to write the complete detial of the calculation because it will be a long paragraph (to get the final value of failure chance, the process doesn't only include the if-then-else of 30-cap and 0-floor) . And I don't think it is misleading, it is true that the formula's result is actually not failure chance, just a medium value. Comparing to the original document which only state the formula, the new edition tells that there are more things to be processed to get the final failure chance value, and also provide a guide to find where the function is, obviously much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, maybe I can just write " the actual calculation doesn't really look like this" in stead of " this is not the resultant value of failure chance".

@KorGgenT
Copy link
Member

KorGgenT commented May 5, 2020

can you also revert all the changes you made to the technomancer toolbelt for now? with nested containers everything that can contain something in magiclysm is going to need to have a serious audit, but i'm waiting for a feature or two before i do so. that is currently the conflict, as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mods: Magiclysm Anything to do with the Magiclysm mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants