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

Please generate differently named interfaces for EventTarget.Self and EventTarget.Any #810

Closed
FNGgames opened this issue Oct 11, 2018 · 10 comments
Assignees

Comments

@FNGgames
Copy link

Hi,

I am coming across this more and more, and I'm really struggling to work around it. I have situations where I need both the bound event and the unbound event from a component at the same time.

// cant use this because it generates 
// 2x ThingListenerComponent,  2x IThingListener and 2x GameThingEventSystem
[Game, Event(EventTarget.Self), Event(EventTarget.Any)]
public class ThingComponent : IComponent 
{
    public float value;
}

Using an attack component as an example: I want to have the attack notify the entity's view that an attack has occured so i can switch its animation state, but i also want to have a blood effect controller sitting around and listening for any attacks so it can spawn in particles, and I want this to be global, not hanging off each entit's view. Can you detect when both of these attributes have been added and add the words Self and Any to the listener components and interfaces and whatnot?

@AVAVT
Copy link

AVAVT commented Oct 12, 2018

Why not just leave it as EventTarget.Any and in the entity's View put a check if(entity == gameEntity)?

@RealCosmik
Copy link

I have also run into this problem to and I think different names would be very useful

@MrXo
Copy link

MrXo commented Oct 17, 2018

@AVAVT This works but is not very convenient. I would also like to have different names for the event listeners.

@FNGgames
Copy link
Author

@AVAVT this sortof defeats the purpose of an event-driven system. The point is to only call the method when necessary. In some cases this would lead to thousands of unneccesary method calls.

@sschmid
Copy link
Owner

sschmid commented Oct 24, 2018

What do you guys think about removing IListener and replace it with

public delegate void OnPosition(Vector3 position);
var listener = contexts.game.CreateEntity();
listener.AddPositionListener(p => transform.position = p);

@roygear
Copy link
Contributor

roygear commented Oct 26, 2018

The content in listener function usually has more than one line code. It is nice to have a distinct function for it.

The one of advantages of IListener approach is that correct function name and parameters can be filled by IDE automatically.

Another personal reason I like to keep using IListener is that I pick the Blueprint up and make it support IListener.

@FNGgames
Copy link
Author

I like the IListener approach for the same reasons, it's nice and convenient in the IDE and it's extremely readable, right at the top of the class you can see what it's reacting to. I just want support for both types of listener at the same time - perhaps the more cumbersome naming (Including the word Self/Any for example) could be reserved only for when the CG detects that both types are defined on one component?

@ghost ghost assigned sschmid Nov 19, 2018
@ghost ghost added the in progress label Nov 19, 2018
@sschmid
Copy link
Owner

sschmid commented Nov 19, 2018

Just submitted PR #827

[Event(EventTarget.Self), Event(EventTarget.Any)]
public sealed class PositionComponent : IComponent
{
    public Vector3 value;
}

will generate IPositionListener and IAnyPositionListener

public void OnAnyPosition(GameEntity entity, Vector3 value)
{
    UnityEngine.Debug.Log(entity.creationIndex + " Position: " + value);
}

public void OnPosition(GameEntity entity, Vector3 value)
{
    UnityEngine.Debug.Log(entity.creationIndex + " Position: " + value);
}

@sschmid
Copy link
Owner

sschmid commented Nov 19, 2018

Need to update roslyn generator and namespace support. Will be in next release.

@sschmid
Copy link
Owner

sschmid commented Nov 19, 2018

Delegates might be more flexible, but currently I also prefer IListener being more explicit.
In order not to break existing code when adding a second event attribute at a later point of time, "Any" will always be prefixed. I think that's a good fit as it describes the intend even more

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

No branches or pull requests

6 participants