-
Notifications
You must be signed in to change notification settings - Fork 272
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
feat(port): start work on porting relic generation #4171
base: main
Are you sure you want to change the base?
Conversation
Co-Authored-By: Curtis Merrill <30208401+KorGgenT@users.noreply.github.com>
Co-Authored-By: Curtis Merrill <30208401+KorGgenT@users.noreply.github.com>
The Autofix app has found code style violation and automatically formatted this Pull Request. I locally edit my commits (e.g: git, github desktop)Please choose following options: I'd like to accept the automated commit
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. This PR is complete and I don't want to edit it anymoreIt's safe to ignore this message. I edit this PR through web UIYou can ignore this message and continue working. I have no idea what this message is talking aboutYou can ignore this message and continue working. If you find any problem, please ask for help and ping @scarf005. |
Please, for the love of all that's holy, improve on the overhaul instead of porting it as-is. Half the artifact effects have been broken or incomplete ever since DDA switched them over to relics, and nobody ever cared to fix any of it. |
I plan to, yeah. Adding more and better relic effects for them to generate with for one, possibly overhauling how the generation tables work. Right now I've kinda left it on the backburner because I need to convert some functions here to use item identity stuff. |
src/map.cpp
Outdated
{ | ||
add_item_or_charges( p, item::spawn( new_natural_artifact( prop ), | ||
calendar::start_of_cataclysm ) ); | ||
add_item_or_charges( p, item::spawn( id->create_item( rules ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the item::spawn call here, you can just use id->create_item raw. It won't cause any real problems but it's spawning a new copy of the item and deleting the original.
Other than that it looks good though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure that's the only problem? Because as it is now it doesn't compile because I haven't finished converting all functions to use item identity stuff, if I recall.
src/relic.cpp
Outdated
|
||
it->overwrite_relic( generate( rules, *it_id ) ); | ||
|
||
return detached_ptr<item>( it ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this needs to be just return it
as well. I'm not sure about the empty error, it's suggesting is_empty. Might be right?
Closing for now as this has been in the "it's not working because I don't fully understand item identity yet" limbo for a while and I've not really made any progress since then. More importantly, an idea struck me last night about a possible way to accomplish this general idea. The good news is it won't involve glomming onto any of Korg's code, trying to untangle the differences in code between the forks, and hunt for any untested jank hidden in it. The bad news is I'm going to it's going to be absolutely fucking tedious for me to work on instead of actually difficult. Will muse upon the idea when I get home. |
So I may be stupid, I'm compile-testing this now and it seems to be working? I'll play with it a bit more and reopen if everything goes well. |
Purpose of change
This finally starts work on porting relic generation.
Very WIP as right now it doesn't compile due to an issue in relic.cpp I'm not sure the issue of, plus other things to be done.
Describe the solution
C++ changes:
item::overwrite_relic
function to item.cpp and item.h.spawn_artifact
into a thing that creates a relic instead of an old-style artifact.JSON changes:
Doc changes:
TODO:
type_weights
and strength stuff into a less convoluted "pick X good values and Y bad ones" system.Describe alternatives you've considered
Putting this off until Olanti or Kheir suffer through trying to port this and test it for bugs instead.
Testing
Testing to continue as implementation ensues.
Of note, current issue:
I'm assuming this means I placed the needed file contents in the wrong spots within namespaces/classes/whatever, due to the fact that #2284 already defined some of the same spaces the relicgen PR defined, I just can't spot the problem at a glance here.
Additional context
Source PR: relic generation, by @KorGgenT: CleverRaven/Cataclysm-DDA#41589
Checklist