I am pondering about the best practices concerning visibility with class members/properties and how to achieve data encapsulation in UE for a while now.
In the examples (like Shooter Game or Strategy Game), many times public members are used. When a member needs to be a UPROPERTY, this is even enforced now: a private UPROPERTY leads to a compiler error (“BlueprintReadOnly should not be used on private members”). This makes sense, since defining it as UPROPERTY looks like the equivalent of making the member public to the Unreal Editor, as far as I understand it. But making the member protected does not produce any error. Why is that? protected basically works like private in making it non-public for other classes not inheriting from the one defining the members.
Aside from that, in a recent post I learned that the recommended way of adding components to classes now is to make them private but readable by Blueprints. This can be achieved using the meta specifier “AllowPrivateAccess”, which disables the compiler error. It seems like a (dirty?) workaround for stuff like components to compromise the visibility at least for the Editor (couldn’t find any documentation sadly). I guess it should not be used frequently to have private members with getters/setters in C++ and directly writable properties in the Editor, at least that approach sounds inconsistent to me.
Am I right in the assumption that the best way to enforce information hiding in UE would be to have private members and getters/setters that are BlueprintCallable UFUNCTIONs so they have to be called from the editor to edit the value?
Or is this not worth the effort? (I’m getting the suspicion I’m being overzealous about best practices here since I see public members used so many times in the examples)
One drawback with using getters/setters is, that I think I can’t use archetype defaults then since the member is not directly editable by the Editor.
What’s your opinion or experience on public members vs private members with getters/setters in UE?
I think you might be reading a bit more into the naming of UPROPERTY than is warranted. While it is true that UPROPERTY does make a field “public” to the engine in the sense that the engine itself can access that object, it is not much different to when the .net CLR for example has access to your private fields for GC purposes.That is in fact a very good analogy since one of the primary purposes of UPROPERTY is to mark fields as in-use for the UE4 garbage collector. That is, if you don’t have any active UPROPERTY pointers to a UObject, that UObject becomes eligable for GC!
It also adds support for other features (serialization etc.).
My understanding is that the name UPROPERTY does not have much, or anything to do with encapsulation, and is more just a metadata marker.
Consequently, I don’t think you need to pay much heed heed to UPROPERTYs when thinking about encapsulation in your class designs. If you want a public setter/getter, make a setter/getter UFUNCTION (or plain function if you don’t want it exposed to BP) under public. If you want private data, put it in private etc. After you’ve designed your class, then you can start to think about which fields need to be adorned with UPROPERTY, either to secure an UObject from GC, or to expose it to the engine / BP, or if you want replication/serialization for example.
I have not seen the post you are talking about, but I don’t think I’ve ever seen an example of making component references private with the AllowPrivateAccess flag. I agree that it does not make much sense, but in the end I would argue that the pros of the UPROPERTY system outweight it’s cons.
I agree with your last assumption, the norm is unfortunately not to create public accessors with private data, most classes in the engine simply expose all UPROPERTY data directly in their public segment. To make matters even more confusing, quite a lot of classes do both! That is, they both have an accessor function AND expose the data that the accessor retrieves for you. In some cases this is also dangerous since accessing the public field directly can lead to unexpected behaviour (FHitResult.HitActor is a good example of this, as you should always do FHitResult.GetActor()).
All in all though, when it comes to blueprint, there are more problems than just the encapsulation/design problems you are bringing up. But I think it is important to realize your goals. If your focus is to enable easy iteration for your designers, then taking the encapsulation hit might be worth it. If you are more concerned about good design and implementing most of your logic in C++, putting more emphasis on the design might be preferable.
The short of it is that you want to make components private in order to prevent them being overwritten accidentally. This used to be prevented by the TSubobjectPtr template which limited assignment of subobjects (such as components) to the constructor, where such subobjects are meant to be initialized. Since that was deprecated, the need arises to hide components.
Furthermore, 4.7 started enforcing visibility of properties at a blueprint level, so that you don’t end up setting a private property as BlueprintReadWrite and end up overwriting it in blueprint instead of code. But this contradicts the requirements for components to be marked as visible, hence the AllowPrivateAccess keyword.
Yes, you could use AllowPrivateAccess on anything, but that’d be shooting yourself in the foot by defeating the purpose of encapsulation.
As for the actual best practices of encapsulation, C++ and UE4 is no different from any other language – you should encapsulate fields that aren’t meant to be used directly. For instance, if a member variable has dependent data that would need to be updated in turn, then you should encapsulate that member variable so that your setter can update said dependent data.
In C++, this is best done sooner than later, because unlike C# and its built-in attribute keyword, C++ setters/getters require different syntax and going through your entire project to change a common variable like “Mesh” to GetMesh() and SetMesh() is bothersome to say the least. Though Visual Assist does a half decent job of it using the encapsulate member refactoring option.
I’m probably missing something, but when I fished about in the source after I first heard about the use of AllowPrivateAccess, I could only find a handful of classes employing it, which I though made a lot of sense.
Given your feedback, I thought I might have had an older version of 4.7, and maybe that there were more usages in the later commits. But pulling latest and doing some more greps (on bdc20b35fb6b3f5ac83bdda9e93f8c15311f2aa2 to be exact), I can still only see it being used in about 40 or so files out of the hundreds in the engine, and most of the usages are in very obscure classes, with a few notable exceptions in Character.h and DefaultPawn.h.
What you mention next makes a lot of sense however. It is an interesting compromise to allow us to make private, BP exposed UPROPERTYs, but I can’t help but feel that the difference between something like this from DefaultPawn.h:
With the header tool understanding the (meta = (AllowPrivateAccess = “true”) implicitly since it is under the private section. I guess there is some merit to having it be explicit since the norm is probably to not expose a private property to BP, but personally, I think it adds more confusion than clarity if that is indeed the goal as we already have visibility keywords.
In your last paragraph, I am not sure which attribute keyword in C# you are referring to. I assume you mean auto properties, but these are defined with the {get; set;} shorthand and not a keyword. There is indeed the ‘property’ keyword in C++/CLI for the same purpose, maybe that is what you were thinking about? I suppose that is a minor detail, but I thought it could be worth mentioning
If it’s not explicit, then how do you determine when a private property is really meant to be exposed to the editor and blueprint? Just make a special exception for actor components? If so, then how do I do the same for my own subobjects? Surely some metadata would be required and… why not just use the same metadata all over?
It seems like the best compromise to me. What might be more confusing is the sudden change in enforcing visibility of properties on a blueprint scope. But that’s just natural encapsulation to me.
Mixing up my terminology, it’s been a while since I did C#. Attributes are a wholly different thing, yeah.
I meant that properties in C# use the same syntax as direct member variable accesses. So if you later find out that you need to wrap a member variable with a getter/setter, you don’t need to hunt down all references to the variable in source code to replace them with GetVariable() and SetVariable( foo ). Which isn’t too bad with regexes if your variable has a unique name, but a bloody nightmare if it’s common like “Mesh”.
Oh, I did not mean that a special exception should be made for certain types, only that we already have a wealth of visibility specifiers when it comes to UPROPERTY syntax in the ObjectBase.h file. I was merely suggesting that instead of the AllowPrivateAccess being part of the meta=X section, I think it would be less confusing if it instead would be a part of the Visible/Editable & BlueprintX syntax that we already have. I wasn’t trying to make a concrete suggestion for a change, I was just a bit surprised that they opted for making it a meta-tag. If I were to make a concrete suggestion (But since I’ve only spent a couple of minutes thinking over this suggestion, I wouldn’t be surprised if it has some flaws), then maybe something along the lines of omitting the BlueprintX keyword when you are defining a private UPROPERTY would make it not visible to BP, and if any of the BlueprintReadOnly/BlueprintReadWrite combinations with Edit/Visibility keywords are defined, this would make it available. I think something like this might be more intuitive, since it would leverage something that people are already familiar with instead of creating a new concept.
In the end however, I think the important takeaway is that we already have the option of using private UPROPERTYs as a substitute for TSubobjectPtr, which is great
Ah yes, I completely agree with this sentiment :). As a side note, I generally prefer longer, more specific names on my fields/properties. Both to be more clear of their purpose, but also to make grepping / refactoring easier. This is of course not always appropriate however, so your point is still very valid.
@: You’re right, I got some things mixed up. Thanks for explaining! I also confused editor editability (EditAnywhere, EditDefaultsOnly) with Blueprint readability (BlueprintReadWrite, BlueprintCallable). Now I see they mean different things and it’s quite possible to have a private property that can be set by the editor, but which is not directly callable by a Blueprint. I do this through a getter now.
Initially I forgot to add a link to the post I was referring to. I edited my first post later and added the link, but I see already posted in this thread to answer this question.
@: Thank you very much for your advice! You are right and I already thought so, but sometimes it’s nice to get some confirmation.
as we already have visibility keywords.