Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#409 closed task (fixed)

Simplify C++ QueryInterface syntax

Reported by: Philip Taylor Owned by: Philip Taylor
Priority: Nice to Have Milestone:
Component: UI & Simulation Keywords:
Cc: Patch:

Description

Currently C++ components have to say:

ICmpPosition* cmpPosition = static_cast<ICmpPosition*>
  (context.GetComponentManager().QueryInterface(GetEntityId(), IID_Position));

which is horrid.

JS just says

var cmpPosition = Engine.QueryInterface(this.entity, IID_Position);

which is nicer. C++ has got to have more type declarations, and there's tradeoffs with other desirable features of the code, but it should be possible to do better than the current syntax.

Change History (9)

comment:1 by Philip Taylor, 14 years ago

Milestone: UnclassifiedSimulation stage 1
review_request: 0

comment:2 by Philip Taylor, 14 years ago

An obvious thing to try:

#define QUERY_INTERFACE(ent, type) static_cast<ICmp##type*> (context.GetComponentManager().QueryInterface((ent), IID_##type));

CmpPosition* cmpPosition = QUERY_INTERFACE(GetEntityId(), Position);

which makes the assumption that context is available. Under the current model, usually it's an argument to the current method, so that's fine; but sometimes you'd need to do CSimContext& context = m_Context; first, which is nasty since the use is hidden in the macro. Unless we change the way the context is passed around within components, the macro should probably be:

#define QUERY_INTERFACE(context, ent, type) static_cast<ICmp##type*> (context.GetComponentManager().QueryInterface((ent), IID_##type));

CmpPosition* cmpPosition = QUERY_INTERFACE(context, GetEntityId(), Position);

so it's explicit.

An alternative is to have a real entity object, instead of entity IDs, and the object can provide methods like ICmpPosition* GetPosition() (probably still implemented with the global interface/component maps - the entity object itself wouldn't contain any data (other than the ID and maybe a pointer to the component manager), and wouldn't be reference-counted or anything).

In JS, entity objects mean you could write var cmpPosition = this.entity.Position. (That's independent of whether the C++ implementation has entity objects or entity IDs). But I don't really like how it makes the relatively expensive map lookup look like a simple property access; the explicit QueryInterface discourages its use, without being painful when you really do want to use it.

comment:3 by Jan Wassenberg, 14 years ago

  • The basic idea of a macro sounds reasonable, it'd be nice to get rid of some of the duplication.
  • However, specifying Position instead of IID_Position reduces the number of characters, but probably requires *more* typing (IID_* allows autocompletion). there's also the issue of being able to look up the symbol defintion (e.g. to see what other IIDs there are), which is always an argument against token-pasting via preprocessor
  • I wouldn't have any problem with context being an implicit parameter (not exactly a rarity when macros are used), since it seems likely for it to be called that (I'd expect m_context to be rarer). Maybe we could just create a second QUERY_INTERFACE_FROM_CONTEXT(context, ent, type) macro?
  • The argument that 'entity.Position' is misleadingly cheap/normal is a good one, I wouldn't mind writing QueryInterface in JS.

comment:4 by Philip Taylor, 14 years ago

Yeah, autocompletion would be nice. m_Context is quite common in the current design, since it's used whenever a method (other than those inherited from IComponent) needs to access other components, so I'd prefer not to make that case harder.

olsner suggested something like

ICmpPosition* cmpPosition = ICmpPosition::GetFrom(context, GetEntityId());

Then there's also

class IComponent {
    template <typename T>
    T* QueryInterface(context, ent) {
        return static_cast<T*>(context.GetComponentManager().QueryInterface(ent, T::GetInterfaceId()));
    }
    template <typename T>
    T* QueryInterface(context) {
        return QueryInterface<T*>(context, GetEntityId());
    }
};

...

ICmpPosition* cmpPosition = QueryInterface<ICmpPosition>(context);
ICmpPosition* cmpPosition = QueryInterface<ICmpPosition>(context, otherentity);

I think that'd work, and it's about as concise as possible, and it doesn't need macros, and the default-to-self is convenient (and a common case).

comment:5 by Jan Wassenberg, 14 years ago

Nice! I like the template idea more than a macro - it's just regrettable that argument deduction doesn't work. It also seems better than the GetFrom suggestion (as discussed in the meeting, that would have to be defined by a macro and can't be in the base class and is therefore a little less obvious).

comment:6 by Philip Taylor, 14 years ago

Deducing T from the return type, in the variable initialisation? I suppose that could actually work if the type wasn't a raw pointer... If not using raw pointers, perhaps something like:

template <typename T>
class CmpPtr {
    T* m;
public:
    CmpPtr(context, ent) {
        m = static_cast<T*>(context.GetComponentManager().QueryInterface(ent, T::GetInterfaceId()));
    }
    T* operator->() { return m; }
};

...

CmpPtr<ICmpPosition> cmpPosition(context, GetEntityId());

That can't do default-to-self but it means you only have to specify the interface type once. But maybe IDEs can't autocomplete method names after you type cmpPosition-> when it's not a real pointer.

There's too many options to choose from :-(

comment:7 by Jan Wassenberg, 14 years ago

No worries, even VA 10.2 on VC2005 is able to autocomplete class members of smart pointers (including the basic CmpPtr you sketched above). I think this new idea improves upon the previous ICmpPosition* cmpPosition = QueryInterface<ICmpPosition>(context);

Writing GetEntityId() isn't particularly onerous, and this solution avoids any and all redundancy without macro hackery.

comment:8 by Philip Taylor, 14 years ago

Resolution: fixed
Status: newclosed

I was more worried about Eclipse, since its autocomplete is far less good than VA's. But it seems to cope fine here. I've done the CmpPtr thing now (see changeset 6a8a0491d4a2), so hopefully it won't need many more changes.

comment:9 by (none), 14 years ago

Milestone: Simulation stage 1

Milestone Simulation stage 1 deleted

Note: See TracTickets for help on using tickets.