Needless finalizing of virtual functions.

Development Programming & Scripting
,
1

I’d love for an engine dev to explain why some C++ functions (seemingly randomly) go out of their way to get overridden with final in a derived class when their base class specifically declared them as virtual so that they could be overridden?

For example, SWidget implements a nice virtual function for SetVisibility.

/* SWidget.h */

SLATECORE_API virtual void SetVisibility(TAttribute<EVisibility> InVisibility);

/* SWidget.cpp */
void SWidget::SetVisibility(TAttribute<EVisibility> InVisibility){
	VisibilityAttribute.Assign(*this, MoveTemp(InVisibility));
}

This is super useful and can be overridden if for example you need/want to run some logic prior to, or the visibility change occurring.

Then for no logical reason, in SCompoundWidget (commonly derived from) UE decided to finalize an override that just calls the base SWidget function… Why

/* SCompoundWidget.h */
	SLATECORE_API virtual void SetVisibility(TAttribute<EVisibility> InVisibility) override final;

/*SCompoundWidget.cpp*/
void SCompoundWidget::SetVisibility(TAttribute<EVisibility> InVisibility){
	SWidget::SetVisibility(MoveTemp(InVisibility));
}

I understand why one would choose to finalize a function, but more often than not, such as in SCompoundWidget there appears to be no good reason for it as the finalized override just calls the base implementation anyway.

I’m aware of the UUserWidget.OnVisibilityChanged event, but that only helps blueprint devs, and not CPP devs who may have needed their widget to inherit from UPanelWidget instead. Even then it’s only for UMG ‘post change’ events and not Slate.

The workaround would be to just or fix the engine source... or declare a SetVisibility_MySlateWidget function that does it’s work then calls the underlying function, but that just clutters the API and opens the door for maintenance and use errors.

This isn’t the first time I’ve encountered this design decision by the engine team for seemingly zero benefit either. As a minimum if finalize was actually needed/important they could comment why they ‘needed’ to block the function.

2

I doubt the developers who programmed this are out there somewhere. This function must have been written a very long time ago, so there probably isn’t anyone here who can explain exactly why it’s done this way. I checked the source code briefly, and it looks like all the child functions (except SNullWidget) just call the parent class function, so making it virtual doesn’t even make much sence to me, to be honest.

I think (and I could be wrong) that they made it final for all children because they thought overriding would never be necessary. A function, based on its name, should do exactly what it’s named for, and nothing more. Adding extra behavior to a function will actually create more confusion, since people won’t expect it to behave differently when overridden. If you want to add extra logic to it, you’ll probably have to create a new function and name it to reflect the new functionality. With the added logic, it won’t be just SetVisibility anymore, it will need a more proper name.

But these are just my thoughts. I think if I left the project and came back to it some time later, I wouldn’t even remember that I somehow modified SetVisibility function. If there is a bug (problem) in this function related to the new functionality, it will take me a while to figure it out. So I would say that actually overriding this function will most likely lead to maintenance and use errors (in the long run).

3

What you say with regards to naming makes sense on the surface. While one certainly wouldn’t want SetVisibility to also change the color, it’d be nice for the ability to fix or enhance missing functionality when inheriting without having to get into engine modification.

Every time I’ve run into issues with final, it’s because the slate/UMG team decided to stray from the pattern requiring workarounds on top of workarounds just to get the system to perform as expected/desired.

In the end it’s another frustration with UE, hitting roadblock after roadblock after a feature has been effectively finished. The epic devs trust us enough to modify the engine’s source, but not enough to override a virtual function…

New week, New cursing, but hey at least I’m still maintaining 90fps in a VR app on target hardware (6 year old GPU). Should work great on a modern GPU.

4

Technically you could just override the SWidget class, create an SMyCompoundWidget and make it just like SCompoundWidget, except now you’d be able to implement SetVisibility without the final keyword. Of course, you probably don’t want to do that, but it might be your only option to modify that function without overriding the engine.

I had a similar problem once, but even worse than yours. I wanted to override the STextBlock class, but the most important variables (almost all class variables) I needed were private with no getter functions to access them. So while I could create a child widget from that widget, I couldn’t override pretty much anything. The functions that contained those private variables were marked as virtual override and didn’t have the final keyword, as if I could override them, but I couldn’t do anything about it.

It’s not that the devs are trying to prevent you from doing something just because they don’t want us to have access to very important code. The barriers are there because of a decision they thought was right when they wrote the code. In the same way, one could argue why they would use the private/protected keywords, since that would likely also create limits in some cases. But I agree with you, sometimes these decisions may not be entirely justified.