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

Start and Stop signal registration for the FlaggedStorage #613

Merged
merged 3 commits into from Sep 3, 2019

Conversation

AndreaCatania
Copy link
Contributor

Sometimes you may want to perform some operations on the storage, but you don't want
that these operations produce any event.

You can use the function storage.set_event_emission(false) to suppress the event writing for
of any action. When you want to re activate them you can simply call
storage.set_event_emission(true)

This change will be used to fix this: amethyst/amethyst#1795

Checklist

  • I've added tests for all code changes and additions (where applicable)
  • I've added a demonstration of the new feature to one or more examples
  • I've updated the book to reflect my changes
  • Usage of new public items is shown in the API docs

@jaynus
Copy link
Contributor

jaynus commented Jul 17, 2019

Wouldn't setting storage.set_event_emission(false) accidentally allow for #541 in some cases?

@AndreaCatania
Copy link
Contributor Author

Do you mean that when the emission is false, the parallel joint is allowed?

I think that this change doesn't fix or make worse that scenario, because the main point of that issue
is find a way to join in parallel the flagged storages and write the events.

So I would not consider this as a fix or a partial fix.

@AnneKitsune
Copy link
Contributor

bors try

@bors
Copy link
Contributor

bors bot commented Aug 1, 2019

🔒 Permission denied

Existing reviewers: click here to make jojolepro a reviewer

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

I'm not really sure if this belongs into Specs. It seems to me this is quite a special case.

@torkleyy
Copy link
Member

torkleyy commented Aug 2, 2019

Sometimes you may want to perform some operations on the storage, but you don't want
that these operations produce any event.

Why do you want that? It seems to me in this case the requirement stems from unclean design. FlaggedStorage is just meant to be a very simple wrapper which just gives you all the events.

If you want a more custom storage, please consider making your own instead of adding it to Specs; I'm trying to keep Specs small, and stabilize it.

@AndreaCatania
Copy link
Contributor Author

The reason is that sometimes a particular System perform some operations on the storage and these don't have to be propagated.
Like the parenting hierarchy resolution of the Transform component in Amethyst here amethyst/amethyst#1795 .

Create a new Storage to solve this problem is doable, but mean also copy an paste 80% of the actual FlaggedStorage.

If you and the others are ok with it, I can create a new storage type that is capable of start and stop event signaling and so solve that issue.

@AndreaCatania
Copy link
Contributor Author

@torkleyy However this functionality is not so invasive, and only add a feature that can be useful even out of Amethyst.

Is always a good thing be able to start and stop event submission.

@AndreaCatania
Copy link
Contributor Author

Hello guys! Any idea about this PR?

@AnneKitsune
Copy link
Contributor

I'm okay with merging this, but if @torkleyy doesn't want to, then it would make sense to create this type of storage in an external crate and use it in amethyst.

The main downside is that using an external crate will require more maintenance.

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Aug 18, 2019

I waited a bit to hear the @torkleyy thoughts on this;
I assume that he don't want to merge it, so I'll create a new crate in the next few days.

(By the way, I think that use a different crate for this, is not the optimal solution in this case)

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

Can we put this under a feature flag? I think it would be fine to merge it when it's disabled by default; the API seems clean enough to do that. Though I still feel more cases will come up where we want to customize FlaggedStorage.

@AndreaCatania
Copy link
Contributor Author

Yes I can to it.

I think it would be fine to merge it when it's disabled by default;

Is it already disabled by default.

Can we put this under a feature flag?

Would you like to use a flag (i32 - bit wise operations) instead of a boolean?

@AndreaCatania
Copy link
Contributor Author

Re-thinking about your comment, probably your idea is to use the conditional building using the features. Let me know which one

@AnneKitsune
Copy link
Contributor

feature flag = "conditional building" = features = ["somethingsomething"]

@AndreaCatania
Copy link
Contributor Author

@Jojolepro Thanks for the clarification!
@torkleyy Done!

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

Thanks!
Take this as my approval after addressing my review comment.

r? @Jojolepro @jaynus

src/storage/flagged.rs Outdated Show resolved Hide resolved
@AndreaCatania
Copy link
Contributor Author

@torkleyy Done, now looks much better.

@AndreaCatania
Copy link
Contributor Author

Morning, any idea about this?

@WaDelma
Copy link
Member

WaDelma commented Sep 3, 2019

bors r+

bors bot added a commit that referenced this pull request Sep 3, 2019
613: Start and Stop signal registration for the FlaggedStorage r=WaDelma a=AndreaCatania

Sometimes you may want to perform some operations on the storage, but you don't want
that these operations produce any event.

You can use the function `storage.set_event_emission(false)` to suppress the event writing for
of any action. When you want to re activate them you can simply call
`storage.set_event_emission(true)`

This change will be used to fix this: amethyst/amethyst#1795

## Checklist

* [x] I've added tests for all code changes and additions (where applicable)
* [x] I've added a demonstration of the new feature to one or more examples
* [x] I've updated the book to reflect my changes
* [x] Usage of new public items is shown in the API docs


Co-authored-by: Andrea Catania <info@andreacatania.com>
@bors
Copy link
Contributor

bors bot commented Sep 3, 2019

Build succeeded

@bors bors bot merged commit 7903f8f into amethyst:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants