Gui: fixes issue #11113: Adjust Default Main Light Position

[wwmayer]

No description provided.

[luzpaz]

Can someone make a clip demonstrating this feature please? 🙏

[wwmayer]

Inside the red circle there is a handle that you can drag and rotate. This changes the direction of the light source.

[xtemp09]

I'm not an expert in 3D modeling, but it would be better if there were a few light sources.

https://www.youtube.com/watch?v=ElMM3u2MO5k&t=97s

[luzpaz]

CC @howetuft (think you would be interested in this feature)
@MisterMakerNL would you mind taking a look at the stylesheet issues of this feature once it's merged ? TIA!

[wwmayer]

I'm not an expert in 3D modeling, but it would be better if there were a few light sources.

I don't think Coin3D is capable of what Blender can do.

I have tested what happens when there are more light sources. When they emit their lights from the sides then your model has almost no dark areas any more but it also increases the risk that it looks completely white.

For now this PR solves the reported issue but it might be an option for the future to extend the lighting.

[MisterMakerNL]

Most 3D CAD programs like Solidworks use HDR maps just like easy reflect macro does. https://forum.freecad.org/viewtopic.php?t=80916&sid=4ce1afafcd81f8607c8cbb504128af98
And then you have indeed also the option for a 3-point light.

I have a huge folder with HDRi studio maps, I don't know the license tho but we can use them to test.
I can also make HDRI in Blender so we don't get any license issues.

[chennes]

I think that this is a clear improvement, so even if there are even more ways of improving, I'm inclined to merge this as-is.

[maxwxyz]

Thanks, I will test it when it is in the weekly builds as I cannot build it myself.

• Is the absolute angle defined (world coordinates) or depending on the camera view (second one preferred)?
• Is the default setting so that it is not parallel / identical to the camera view [Problem] Adjust Default Main Light Position #11113

[wwmayer]

The lighting is done by SoDirectionalLight that has a SoSFVec3f field. I don't think this direction depends on the camera view direction. The default light direction is (0,0,-1). As it's independent of the camera view direction it's only parallel if the camera looks down/up the Z axis.

[maxwxyz]

Then this is not what renders all glossy materials white. If you look at the example of #11113 from the bottom view (or normal to any face) they will render white. I just coincidentally modeled the demo file so that it appears white from the top (could be from any direction)
Edit: I have just rotated the first body 90° around Y, looking from the right view it also appears white without being able to distinguish face colors, selected faces or highlighted faces (mouseover) [in yesterdays weekly build, without your commit]:

Or is (0,0,-1) in reference to the camera view (as Z is always normal to sketches in their coordinates)? Then this would be fixed with the new setting when changing the default.

[wwmayer]

That's why we have the preferences page where you can change the light direction to something different to (0,0,-1). If you then look from top on the model it doesn't look white any more.

Another option is to reduce the intensity from 100% to e.g. 60%.

[maxwxyz]

OK I will test it when available. Couldn't we set the default value then to something coming e.g. from the top left as this would not influence anything badly but improve the default behavior that the faces appear white when looking normal to the camera direction (editing sketches, selecting references,...):

[wwmayer]

If needed we can change the default.

[maxwxyz]

great, I would propose (1, -1, -1) - is this feasible?

[xtemp09]

float QColor::redF() const returns float. SbColor (const float r, const float g, const float b) requires float, however, this fragment of the code converts float into double and then double into float.

QColor color = ui->light1Color->color();
double red = color.redF();
double green = color.greenF();
double blue = color.blueF();
view->getHeadlight()->color = SbColor(float(red), float(green), float(blue));

Is there a reason behind that?

[wwmayer]

great, I would propose (1, -1, -1) - is this feasible?

This doesn't look good. I would try (0.2, -0.2, -1). As already said you can test this setting with your version. Open the parameter editor and go to BaseApp/Preferences/View. If the key HeadlightDirection doesn't exist then create it as a text object and set its value to (0.2,-0.2,-1)

[wwmayer]

float QColor::redF()

With Qt5 It returns a qreal which is an alias for double. But with Qt6 it indeed returns a float now.

[maxwxyz]

great, I would propose (1, -1, -1) - is this feasible?

This doesn't look good. I would try (0.2, -0.2, -1).

This looks good!

[xtemp09]

Why is Q_PROPERTY rare in the source code of FreeCAD? Wouldn't it fit perfectly here, especially with such benefits like notification event, reset etc?

Why do the developers deliberately avoid standard usage of Qt Signals & Slots mechanism, for example, in case of setPaneText of MainWindow?

@wwmayer, this pull request seems to avoid its standard usage too.

Here, you connect the checkbox

connect(ui->checkBoxLight1, &QCheckBox::toggled,
        this, &DlgSettingsLightSources::toggleLight);

with the function

void DlgSettingsLightSources::toggleLight(bool on)
{
    view->getHeadlight()->on = on;
}

while the headlight already has the method toggling light.

FreeCAD/src/Gui/Quarter/QuarterWidget.cpp

Lines 395 to 398 in b70956b

	QuarterWidget::setHeadlightEnabled(bool onoff)
	{
	PRIVATE(this)->headlight->on = onoff;
	}

Sorry to bombard you with questions, but I hope you will shed headlight when you find time.

[wwmayer]

Wouldn't it fit perfectly here, especially with such benefits like notification event, reset etc?

In how far? The property mechanism allows it to set or get a property by string with qtclass->setProperty("myProperty", value) or QVariant value = qtclass->property("myProperty"). If myProperty is registered with Q_PROPERTY and appropriate setter/getter functions they will be called when accessing the property as shown above.

But I don't see how this would fit here because the dialog must react on signals emitted by its child widgets.

Why do the developers deliberately avoid standard usage of Qt Signals & Slots mechanism,

What is the standard usage of Qt Signals & Slots? Do you mean to write a slot function of the form on_widgetname_signalname() so that it will be connected automatically? This way is not recommended any more because it's error-prone and easy to break. A slot function won't be connected if at a later point the name of the widget changes, the signal name changes (e.g. in a newer Qt version) and so on.

And the old-style connection using the SIGNAL/SLOT macros is not recommended either.

In both cases you only may get a warning at runtime and this is tedious to test. And once you made sure it works correctly you can never be sure that it breaks all the sudden in the future.

The recommended and safest way is to use the new style connect with function pointers because there the compiler already tells you if a connect fails. Of course, it's a lot more work but that's the price you have to pay.

for example, in case of setPaneText of MainWindow?

What exactly is your complaint about setPaneText?

while the headlight already has the method toggling light.

Ah thanks. I will prepare a PR to fix this and the color issue.

[xtemp09]

I should have provided more details about void MainWindow::setPaneText(int i, QString text). My question is related to the issue #9544. Looking into the code I found out that the status messages and dimensions in the sizeLabel are set directly, without involving Qt signal & slot system.

I have made the commit xtemp09/FreeCAD@e2c342b to use the Qt messaging system to deliver the dimensions from View3DInventorViewer to the sizeLabel.

I thought it would be better if the widget uses Q_EMIT clearStatus() or Q_EMIT message(msg) instead of

this

FreeCAD/src/Gui/ProgressBar.cpp

Lines 312 to 342 in aaa9629

	void SequencerBar::resetData()
	{
	QThread *currentThread = QThread::currentThread();
	QThread *thr = d->bar->thread(); // this is the main thread
	if (thr != currentThread) {
	QMetaObject::invokeMethod(d->bar, "resetEx", Qt::QueuedConnection);
	QMetaObject::invokeMethod(d->bar, "aboutToHide", Qt::QueuedConnection);
	QMetaObject::invokeMethod(getMainWindow(), "showMessage",
	Qt::/*Blocking*/QueuedConnection,
	Q_ARG(QString,QString()));
	QMetaObject::invokeMethod(getMainWindow(), "setPaneText",
	Qt::/*Blocking*/QueuedConnection,
	Q_ARG(int,1),
	Q_ARG(QString,QString()));
	d->bar->leaveControlEvents(d->guiThread);
	}
	else {
	d->bar->resetEx();
	// Note: Under Qt 4.1.4 this forces to run QWindowsStyle::eventFilter() twice
	// handling the same event thus a warning is printed. Possibly, this is a bug
	// in Qt. The message is QEventDispatcherUNIX::unregisterTimer: invalid argument.
	d->bar->aboutToHide();
	delete d->waitCursor;
	d->waitCursor = nullptr;
	d->bar->leaveControlEvents(d->guiThread);
	getMainWindow()->setPaneText(1, QString());
	getMainWindow()->showMessage(QString());
	}
	SequencerBase::resetData();
	}

[wwmayer]

I should have provided more details about void MainWindow::setPaneText(int i, QString text)

I think such things should be discussed elsewhere as it has nothing to do with this PR.

I have made the commit xtemp09@e2c342b

Isn't this a direct connection, rather than a queued connection?

I thought it would be better if the widget uses Q_EMIT clearStatus() or Q_EMIT message(msg) instead of

It could be checked as alternative.

[maxwxyz]

@wwmayer Thinking about your implementation of the light setting: would it be easy to implement the option in the preferences to add multiple lights, adjust them each and also remove lights again?

[wwmayer]

This could be done in future PR

[maxwxyz]

Done: #11166

[maxwxyz]

@wwmayer when I set the HeadlightDirection in the Parameters Editor directly, it is instantly changed in the 3D view. When I then open the preferences and just klick apply, another value is set.
It seems like the HeadlightRotation instead of the HeadlightDirection is used or what is the workaround?

[wwmayer]

The light dragger node returns a rotation but the directional light node only needs a vector.

Now when opening the preferences the rotation must be restored but from the stored vector alone this is not possible because it's ambiguous. That's why the values of the rotation are additionally saved as quaternion.

If you directly modify the direction vector in the editor it will be overridden by the values that are derived from the rotation the next time you open and save the preferences dialog.