Idea for (new) generic version of CastleComponentFactory

Editor
1

playing with new ways of defining data/props of components for the project(s) I came again to a burden of messing with “AssociateReference”/template when loading components from Factory, it always blew my mind, but somehow I managed do make everything working.

I see this instrument as really helpful, but every time I have to re-analyze the asteroids example to grasp interconnections of all involved elements, and “translate” to my case. Many times I did that Associate and it worked in result, but requires to be really cautious what you do, so I bumped into long debug/adjust sessions with the code (which is also interconnected with other parts of game, not always easily separable as in asteroids example).

So this time I decided to investigate the code behind it and analyzed castlecomponentserialize.pas and factory class. It made it clearer, but also wondering, if it could be done better (to my opinion).

The main goal was to make a factory which would accept a class with extra defined properties (like button with a bunch of images on it, which I want to control via logic in this same class). The standard Associate-approach only defines a set of references which are not really connected to a button, and code looks unnaturally (esp who owns what, how to access, for example, from button onclick those props listed in associate and so on). One smaller additional goal is to make it work independently of “editor with custom components” feature, as it requires rebuilding of Editor with Lazarus.

So I made my own version of a factory, which is generic and allows to load an object which is defined by that T and all its properties are accessible in natural way. So you just write

Factory: specialize TCastleComponentFactoryNew<TMyButton>  ...
myButton := Factory.load(owner)
myButton.CustomImage.Exists := false

and you don’t need any intermediate (like Associate), and even RegisterComponent call (which I was surprised to encounter), and you get all the type safety and power of direct accessing properties (no need to assign or copy smth), your component just needs to resemble the design file.

So, here is the branch where CGE changes are done:

and here is usage example

this is not in any way a direct proposal for the engine, rather than just my thoughts on how it could be alternatively (not replacing) working.

Maybe this would inspire a talk or further investigation :slight_smile:

P.S. A note about topic category: it is not directly Editor topic, bc such approach could have profits just working by code as well. But I think it comes with full power when done via Editor and decent amount of factories in your views.

P.P.S. in my code there are CastleFog components (which might be confusing for 2d-ui game), but I find it as a handy approach to define some additional components’ properties in the editor via just storing them such way (and then getting from code, thus avoiding hardcode). It is again about removing the dependency on custom components.

2

Thanks for the feedback!

I admit I also still have to look up the AssociateReferences usage with TCastleComponentFactory.ComponentLoad. Ideas how to make it more obvious are welcome:)

<sidenote>

The current TCastleComponentFactory.ComponentLoad should not require to rebuild the editor, or to use RegisterComponentEditor or other stuff documented on custom components. The space_shooter example doesn’t use them either.

You only need to use RegisterComponentEditor if the serialized component class, to which you want to refer from the published section, is your own component class. You then need RegisterComponentEditor for all serialization use-cases – whether from TCastleComponentFactory.ComponentLoad , or from global ComponentLoad, TCastleView.DesignUrl etc.

</sidenote>

I looked at what you did, and I like it overall:)

  • I see that effectively user is expected to define a descendant of the root of loaded node, with the desired fields as published.

    TDifficultyButton = class(TCastleButton)
published
  ImageMarkerRight, ImageMarkerLeft: TCastleImageControl;
end;

    I like this part a lot – it looks very clean and obvious. Conceptually, a factory purpose is exactly to return something like this.

  • Then instead of TCastleComponentFactory one declares field with type {$IFDEF FPC_OBJFPC} specialize {$ENDIF} TCastleComponentFactoryNew<TDifficultyButton>.

  • Potential issues I see:

    • (very minor, or even invalid, note) Using one class to get both root node + and the associations risks that you will mix things when they have overlapping fields. That is, if the ancestor, like TCastleButton, would have published field like ImageMarkerRight – it would capture the association. In the existing “AssociateReferences approach”, there’s no such risk, as you always start from simplest TPersistent. I think this is minor “nitpick” note - because our components in practice rather not define published fields, they expose functionality using published properties.

      Upon rethinking, FieldAddress may associate in this case with the descendant field anyway, would make sense (as that’s how Pascal scope also works), so this is likely just non-existing issue.

    • In your implementation, you don’t check if the root node is really of type T, you assume it by T.Create, from what I can see. If it is not (e.g. root was basic TCastleUserInterface and you load it to TDifficultyButton) – we will have wrong class assigned to the variable, with undefined effects. Using as with inherited ComponentLoad would make it safer.

    • (very minor) This assumes one manually will declare published field to access the factory in a view, and decide on its type, since at this point one writes TCastleComponentFactoryNew<TDifficultyButton>. Indeed that’s the only way we work now (developers manually define all fields they want to access), though once we implement this I plan to recommend to move away from this, and then factory will by default be just TCastleComponentFactory as editor/serializer doesn’t know more (hmmm, but maybe we could extend it? possible!). This is minor – in the end, this feature will not exclude the possibility to define some things manually.

    • (very minor) I dislike exposing TCastleJsonReader as I see you had to, because of compiler requirements. You probably dislike it too:) Minor note, as we can solve it in various ways, see below.

I wondered can I enable simple implementation that would address 2 issues above, and as a bonus allow you to do TCastleComponentFactoryNew and potential other experiments at application level. I see that the critical thing you need is ComponentNames – you need this to perform the mapping.

  • So I added in this commit a new method TCastleComponentFactory.ComponentLoadWithMap. This is simple to support (actually makes existing ComponentLoad implementation simpler in my eyes, it focuses on what it should), and universally useful for various stuff that wants to know “everything that was loaded”…

  • …and it should allow to define TCastleComponentFactoryNew with ComponentLoadNew in your application easily. If I think right, it could be done now as:

    function TCastleComponentFactoryNew.ComponentLoadNew(
  const InstanceOwner: TComponent): T;

  // define MakeAssociateReferences as you have

begin
  Result := (inherited ComponentLoadMap(InstanceOwner, ComponentsMap)) as T;
  try
    MakeAssociateReferences(Result, ComponentsMap);
  finally FreeAndNil(ComponentsMap) end;
end;

    (untested! please let me know if this will not really work! :slight_smile: )

    So this avoids engine exposing TCastleJsonReader, and it means you use as (check correctness of T effectively).

  • (btw I renamed ComponentNames → ComponentsMap)

  • And it means you can define it in your own application and enjoy, while we decide whether to add TCastleComponentFactoryNew to the engine. I’m not “saying no”, I’m saying “maybe”, because as said above – I think the TDifficultyButton in your usage looks super-nice, this is exactly how my brain “thinks” about what you get from factory.

1 Like
3

Note: Before it gets widely used, I changed the type of dictionary we use for ComponentsMap (now ComponentMap),

  • from TStringList (which can get inefficient for lots of strings)

  • to TComponentMap, a specialized descendant of TDictionary from Generics.Collections. Generics.Collections is nicely optimized, with hashes for keys. It also seems a bit more natural to use than TStringList – e.g. TryGetValue dictionary method is nice, and it knows the values are always TComponent not any TObject.

Iterating over it looks a bit different, see our procedure MakeAssociateReferences(const ComponentsMap: TComponentsMap); implementation and how it defines Pair to account for both FPC and Delphi.

1 Like
4

Tested this approach, it is not working, unfortunately..

5

Ah, I understand you have “invalid cast” at the line

Result := ComponentLoadWithMap(InstanceOwner, ComponentMap) as T; // invalid cast

Hm, I see – we created TCastleButton, since that class was given in the design JSON, not TDifficultyButton.

  • The only solution I see is to allow to specify “force the root class to be T” for the loading routines in the engine.

    • In your original proposal, this was achieved by just doing T.Create.
    • In the engine, if we add “force the root class to be T”, I would like to check it during deserialization, that $$ClassName in JSON is equal or ancestor of T.

  • Note: We really created TCastleButton, not TDifficultyButton, so you cannot reliably “force” the type like Result := T(...), the result is really not T (doesn’t have extra T fields allocated).

Hm, for now – it means that my ComponentLoadWithMap doesn’t allow you to really do this without modifying the engine. Sorry - I missed that this will be a blocker.

  • We need to add “force the root class to be T” publicly at engine level to allow you to make TCastleComponentFactoryNew.
  • Or just adopt the idea of TCastleComponentFactoryNew in CastleComponentSerialize (so we would not publicly expos the “force the root class to be T” ability, but only use it internally in CastleComponentSerialize unit).

Hm, I need to think :slight_smile: For today, ComponentLoadWithMap isn’t as useful to you as I thought – to implement TCastleComponentFactoryNew you still need to fork CGE, as you had. Sorry, I don’t have a better answer now:) I’m leaning now to just add TCastleComponentFactoryNew to the engine (just need a more descriptive name for class – TCastleComponentFactoryCustomRoot? this sounds convoluted and not helpful :slight_smile: ). Too bad FPC doesn’t yet have generic methods in non-generic classes, and we need to invent class for this.

Maybe we simply should add non-generic function like

function ComponentLoadCustomRoot(const InstanceOwner: TComponent; const T: TComponentClass);

It can then be used either to implement generic TCastleComponentFactoryNew or it can be used directly like

MyButton := MyFactory.ComponentLoadCustomRoot(..., TDifficultyButton) as TDifficultyButton;

So the latter looks ugly, repeating TDifficultyButton, but otherwise is equivalent to your idea.

6

Yes, sorry, I didn’t specify (I also found it during my tests) that exception comes from the fact that deserialiser explicitly creates CastleButton(or class defined in JSON file) with any approach(old or with map exposed, contrary to T.Create which I used). And any type of cast would fail bc we need upcast, but fpc only supports downcast.

Whichever looks aligned to your view, I’m ok with any decision, even not doing anything (as I had already the same button functionality within old approach, in master branch). This was my idea to make some “inspiration push”, it might give off results or not, that’s totally ok, it is not a feature request from me :slight_smile:

For the name I had a variant “TCastleComponentFactoryGeneric“, it kinda shows quite explicitly the idea behind it :smiley:

P.S. and yes, I know you care for the safety of this approach overall, and ideally code needs to check for the type compatibility (T to the class defined in JSON, with some kind of InheritsFrom check, accompanied with a method like CreateComponentFromJson but without creation, just obtaining the class), which my approach doesn’t have. So it requires time and mental effort to think about all these means in balance. And again, no push, no rush, just an inspiring idea/discussion.

1 Like