-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added genomes to Edible Flora #5
Conversation
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.
All in all, I think that the implementation is fine. I have no issues with the structure of the code, simply with its location.
After looking through I still believe that most of this PR should actually reside within SF. I have marked the key methods with review comments.
My reasoning for this like I mentioned on Discord, is that EdibleFlora should just have content. This means, essentially, things that have a direct in game presence. So the genome stuff is not "content" it is a system. And therefore should reside in SimpleFarming. It will also likely result in a smoother/easier time integrating as you don't have to worry about data passing between two different modules, let along two different systems.
Finally, if a developer wishes to make use of SimpleFarming for their own module & wants genome integration, they will either have to duplicate code from EdibleFlora or depend on EdibleFlora flora. Neither of these are good options.
*/ | ||
@ReceiveEvent | ||
public void addGenomeRetentionEvent(AddGenomeRetention event, EntityRef entity){ | ||
RetainComponentsComponent retainComponentsComponent = new RetainComponentsComponent(); |
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.
This both seems mildly unnecessary, because if the only thing RetainComponentsComponent
is ever used for is GenomeComponent
then why does it need to exist. Wouldn't a simple flag work. Or even just the presence of GenomeComponent
Secondly, this can go in SF fine
@ReceiveEvent | ||
public void onTransferGenomeEvent(TransferGenomeEvent event, EntityRef bush, BushDefinitionComponent bushComponent, GenomeComponent genomeComponent) { | ||
event.getTransferEntity().addOrSaveComponent(genomeComponent); | ||
} |
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.
Should be in SF
* @param plant the plant that is being planted | ||
*/ | ||
@ReceiveEvent | ||
public void onBeforePlantedEvent(BeforePlanted event, EntityRef plant) { |
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.
This could be in SF
} | ||
|
||
@Command(shortDescription = "Prints genome of held item if possible.") | ||
public String heldGenomeCheck(@Sender EntityRef client) { |
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.
As per discord discussion, this will go into genome itself
* @param creator the creator(bush) of the produce | ||
*/ | ||
@ReceiveEvent | ||
public void onProduceCreated(ProduceCreated event, EntityRef creator) { |
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.
So some elements of this could go into SF. In particular what I think would work better would be to store information about the specific crops possible genomic stuff as a component on its prefab. Then have SF read that and use the information there to mutate the crop as needed
After the review, the content of the Genome Authority system has been moved to SimpleFarming and therefore this PR can be closed |
added genomeComponent to Bushes and seeds and created GenomeAuthoritySystem
To-do before merging