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

Allow trades to be configurable #15

Closed
ErikaRedmark opened this issue Dec 26, 2016 · 7 comments
Closed

Allow trades to be configurable #15

ErikaRedmark opened this issue Dec 26, 2016 · 7 comments
Assignees

Comments

@ErikaRedmark
Copy link

ErikaRedmark commented Dec 26, 2016

I have some other mods running and would like to add to the trades of existing professions. I looked at the code and it seems like this isn't possible as the professions are hardcoded. Perhaps it would be possible to have profession trades set up from .json files?

Going even further I would recommend merging all the Pro[name] Profession classes into just Profession and having different instances of profession loaded from some descriptor file (but that would be like down the road refactoring) as that may even make it possible to define new professions easily.

The reason I am suggesting this is because there are so many cool mods and I like seeing interaction between them, and I think this mod would, if trades were customisable from .json or some other descriptor format, (and maybe even professions/upgrades), offer really good interaction with pretty much all mods to any mod pack creators wishing to better integrate all the mods to work together. For instance, the carpenter can get bibliocraft blocks and the miner can get custom ores.

But, for the purposes of just this particular issue, I would like to ask for the trade options for the professions to be configurable. That already would add a good level of mod integration depth. Basically, put the items traded, and what they are traded for, in the hands of the user, whilst providing the current trades as defaults.

@ErikaRedmark
Copy link
Author

Just FYI, I got an Eclipse setup and cloned the source code. I am going to try to solve this in two parts: refactoring the subclasses down to a single class with an associated builder class (which would be one pull request) and from there it will be a great deal easier to create Professions from a .json file.

I really like the concept of this mod and I really feel this would massively help it's integratability into other mods. I should point out though I am very unfamiliar with minecraft mods (this is the first work I am doing that has anything to do with minecraft) and what I am doing right now is more of a general design fix to accommodate the changes. At the very, very least, I will make a pull request for the refactoring when I am done and tested it, even if it won't actually solve the issue on its own.

@ckhgame
Copy link
Owner

ckhgame commented Jan 2, 2017

Hi ErikaRedmark, I'm sorry I didn't see your comments until now because I was spending most of time working on another game project.....
I really appreciate your interests in my mod and all the efforts you have done. I'm very willing to upgrade the mod for making the trading configurable.
It may take 1 week to add the code and walk through a simple test. Would it be too long for you?

Thanks! Happy new year :)

@ckhgame
Copy link
Owner

ckhgame commented Jan 2, 2017

Got a good progress tonight, so it won't be one week long!
And I have another good news, I can possibly make all professions be configurable(including upgrading options, trading recipes, quest, etc..).
Its really a good idea to merge all professions in just one and set up all professions through a Json file. thank you :D

@ckhgame
Copy link
Owner

ckhgame commented Jan 2, 2017

By the way which Minecraft version your are using?

@ckhgame
Copy link
Owner

ckhgame commented Jan 2, 2017

Done! ⛏ . I will go through a simple test with @wilusy then release the build!

@ckhgame ckhgame self-assigned this Jan 2, 2017
@ckhgame
Copy link
Owner

ckhgame commented Jan 2, 2017

https://minecraft.curseforge.com/projects/village-box/files
build uploaded, it's under review now, will be soon available....
Thank you again!

@ErikaRedmark
Copy link
Author

ErikaRedmark commented Jan 3, 2017

I was sleeping through the first couple messages and didn't really get a chance to get back online until soon after you already marked this as closed; from the looks of the code it doesn't look like there is anything in what I did that can be added anymore (I wouldn't have known how to specify items from other mods in json anyway, all I was focusing on at the time was removing the subclasses)

In either case, I'll update my modpack as soon as I get off of work and add some trades for some other mods and get a chance to test myself over the week as time permits. Thank you for updating the mod with this feature. Sorry for the delays getting back to you; it was the holiday vacation for me and it just ended yesterday so I was preoccupied.

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

No branches or pull requests

2 participants