kpovmodeler not working with povray 3.7 #2

Злито
SlavekB злито 1 комітів з bug/2765/povray3.7 до master 3 роки тому
deloptes прокоментував(ла) 6 роки тому
Співавтор

A small fix to make it work with povray3.7

https://bugs.trinitydesktop.org/show_bug.cgi?id=2765

A small fix to make it work with povray3.7 https://bugs.trinitydesktop.org/show_bug.cgi?id=2765
SlavekB прокоментував(ла) 6 роки тому
Власник

Povray 3.7 does not allow radiosity? Or is it necessary to use other option?

In any case, this patch needs further work.

Povray 3.7 does not allow radiosity? Or is it necessary to use other option? In any case, this patch needs further work.
SlavekB додано PR/not-ok з міткою 6 роки тому
deloptes прокоментував(ла) 6 роки тому
Автор
Співавтор

AFAIR this option is gone

AFAIR this option is gone
deloptes прокоментував(ла) 6 роки тому
Автор
Співавтор

http://www.povray.org/documentation/3.7.0/r3_2.html#r3_2_8_8

as I mentioned in the original bug a lot needs to be reworked, but the purpose of this patch is to make it at least rendering the model, as otherwise it renders the software mostly useless.

regards

http://www.povray.org/documentation/3.7.0/r3_2.html#r3_2_8_8 as I mentioned in the original bug a lot needs to be reworked, but the purpose of this patch is to make it at least rendering the model, as otherwise it renders the software mostly useless. regards
SlavekB зробив запит змін 3 роки тому
SlavekB додав коментар
Власник

Because it is no longer necessary to deal with compatibility with older versions of povray, so there is no need to leave the code.

Because it is no longer necessary to deal with compatibility with older versions of povray, so there is no need to leave the code.
kpovmodeler/pmrendermode.cpp Застарілі
// if( m_radiosity )
// cl.append( TQString( "+QR" ) );
// else
// cl.append( TQString( "-QR" ) );
SlavekB прокоментував(ла) 3 роки тому
Власник

Instead of leaving the code, it is now appropriate to delete the code, including related GUI options.

Instead of leaving the code, it is now appropriate to delete the code, including related GUI options.
deloptes позначив обговорення завершеним
deloptes примусово залито bug/2765/povray3.7 з %!<(string=8bcebad8ec)/code> до 23b8e49047 3 роки тому
deloptes попросив рецензію від SlavekB 3 роки тому
MicheleC зміни затверджено 3 роки тому
MicheleC додав коментар
Власник

Looks ok to me. Will wait for final approval from Slavek since he has followed this from the beginning.

Looks ok to me. Will wait for final approval from Slavek since he has followed this from the beginning.
SlavekB зробив запит змін 3 роки тому
SlavekB додав коментар
Власник

It seems insufficient for me. My idea is to advance and remove the Radiosity checkbox from the GUI and the corresponding code. Now there is in the GUI checkbox that has no effect at all, which seems confusing.

It seems insufficient for me. My idea is to advance and remove the Radiosity checkbox from the GUI and the corresponding code. Now there is in the GUI checkbox that has no effect at all, which seems confusing.
MicheleC прокоментував(ла) 3 роки тому
Власник

There is indeed extra code related to radiosity, I also noticed this morning. The key point is to make sure we remove what is no longer required and whether to do it in this PR or as a separate one, while using this PR only to avoid setting unused (unsupported?? - I don't know) parameters anymore.

There is indeed extra code related to radiosity, I also noticed this morning. The key point is to make sure we remove what is no longer required and whether to do it in this PR or as a separate one, while using this PR only to avoid setting unused (unsupported?? - I don't know) parameters anymore.
deloptes прокоментував(ла) 3 роки тому
Автор
Співавтор

Sorry, I never knew there is option for radiosity in the GUI.

Sorry, I never knew there is option for radiosity in the GUI.
SlavekB прокоментував(ла) 3 роки тому
Власник

Sorry, I never knew there is option for radiosity in the GUI.

I think the respective checkbox is here: pmrendermodesdialog.cpp:350. At the same time, there will be unnecessary methods setRadiosity(...), radiosity() and variable m_radiosity.

> Sorry, I never knew there is option for radiosity in the GUI. I think the respective checkbox is here: [pmrendermodesdialog.cpp:350](src/branch/master/kpovmodeler/pmrendermodesdialog.cpp#L350). At the same time, there will be unnecessary methods `setRadiosity(...)`, `radiosity()` and variable `m_radiosity`.
deloptes прокоментував(ла) 3 роки тому
Автор
Співавтор

Sorry, I never knew there is option for radiosity in the GUI.

I think the respective checkbox is here: pmrendermodesdialog.cpp:350. At the same time, there will be unnecessary methods setRadiosity(...), radiosity() and variable m_radiosity.

yes, obviously there is more work to do. I will have a look, update the code and request review again.
I'll change this to WIP

> > Sorry, I never knew there is option for radiosity in the GUI. > > I think the respective checkbox is here: [pmrendermodesdialog.cpp:350](src/branch/master/kpovmodeler/pmrendermodesdialog.cpp#L350). At the same time, there will be unnecessary methods `setRadiosity(...)`, `radiosity()` and variable `m_radiosity`. yes, obviously there is more work to do. I will have a look, update the code and request review again. I'll change this to WIP
deloptes додано PR/wip з міткою 3 роки тому
deloptes додав 1 коміт 3 роки тому
deloptes видалено PR/wip PR/not-ok з мітками 3 роки тому
deloptes попросив рецензію від SlavekB 3 роки тому
SlavekB рецензовано 3 роки тому
SlavekB додав коментар
Власник

It looks good, I propose one editing regarding modes 9, 10 and 11.
See comment below.

It looks good, I propose one editing regarding modes 9, 10 and 11. See comment below.
I18N_NOOP( "9: Compute media" ),
I18N_NOOP( "10: Compute radiosity but no media" ),
I18N_NOOP( "11: Compute radiosity and media" )
I18N_NOOP( "9: Compute media" )
SlavekB прокоментував(ла) 3 роки тому
Власник

According to documentation, modes 10 and 11 remain available, however, they seem to be identical to 9. I propose to do an item as such as 6, 7… => give 9, 10, 11… in one item and update description by documentation. The description could also be updated for item 6, 7….

See https://wiki.povray.org/content/Reference:Tracing_Options#Quality_Settings

According to documentation, modes 10 and 11 remain available, however, they seem to be identical to 9. I propose to do an item as such as *6, 7…* => give *9, 10, 11…* in one item and update description by documentation. The description could also be updated for item *6, 7…*. See https://wiki.povray.org/content/Reference:Tracing_Options#Quality_Settings
MicheleC прокоментував(ла) 3 роки тому
Власник

Sounds like a good idea.

Sounds like a good idea.
deloptes додав 1 коміт 3 роки тому
deloptes додав 1 коміт 3 роки тому
deloptes прокоментував(ла) 3 роки тому
Автор
Співавтор

I also think c_qualityToIndex needs to be updated accordingly and I hope the last change does this correctly. So if OK, I will squash the commits.

I also think c_qualityToIndex needs to be updated accordingly and I hope the last change does this correctly. So if OK, I will squash the commits.
deloptes додав 1 коміт 3 роки тому
deloptes прокоментував(ла) 3 роки тому
Автор
Співавтор

I think this last commit is also necessary. I do not understand why it would return numQuality-1

I think this last commit is also necessary. I do not understand why it would return numQuality-1
deloptes попросив рецензію від MicheleC 3 роки тому
SlavekB зміни затверджено 3 роки тому
SlavekB додав коментар
Власник

It looks good. Everything seems to me okay.

It looks good. Everything seems to me okay.
MicheleC зробив запит змін 3 роки тому
MicheleC додав коментар
Власник

Please check and clarify the comment below.

Please check and clarify the comment below.
index = 0;
if( index >= numQuality )
index = numQuality - 1;
if( index > numQuality )
MicheleC прокоментував(ла) 3 роки тому
Власник

Is this change correct? Old code was setting the limit of the index to "numQuality - 1", while the new code is going up to "numQuality" and then using the value to access the array.
If we call the function indexToQuality() with argument value equals to "numQuality" we are going to do an invalid access to the element past the last in the array.
The original code seems correct to me.

Is this change correct? Old code was setting the limit of the index to "numQuality - 1", while the new code is going up to "numQuality" and then using the value to access the array. If we call the function indexToQuality() with argument value equals to "numQuality" we are going to do an invalid access to the element past the last in the array. The original code seems correct to me.
SlavekB прокоментував(ла) 3 роки тому
Власник

Yes, you seem to be the truth – numQuality mentions the size, so the index must be one smaller.

Yes, you seem to be the truth – `numQuality` mentions the size, so the `index` must be one smaller.
deloptes позначив обговорення завершеним
deloptes примусово залито bug/2765/povray3.7 з %!<(string=1f1a0cb713)/code> до 72d4668eea 3 роки тому
deloptes попросив рецензію від MicheleC 3 роки тому
deloptes прокоментував(ла) 3 роки тому
Автор
Співавтор

@MicheleC thank you, I have overlooked this.
Now fixed this and squashed into one commit.

@MicheleC thank you, I have overlooked this. Now fixed this and squashed into one commit.
MicheleC зміни затверджено 3 роки тому
MicheleC додав коментар
Власник

Looks good now.

Looks good now.
MicheleC прокоментував(ла) 3 роки тому
Власник

@MicheleC thank you, I have overlooked this.
Now fixed this and squashed into one commit.

no worries 😄

> @MicheleC thank you, I have overlooked this. > Now fixed this and squashed into one commit. no worries :smile:
SlavekB зміни затверджено 3 роки тому
SlavekB додав коментар
Власник

Thanks, now all seems good.

Thanks, now all seems good.
SlavekB Злиті коміти 72d4668eea в master 3 роки тому
SlavekB видалена гілка bug/2765/povray3.7 3 роки тому
SlavekB додав(ла) до R14.0.11 release етапу 3 роки тому

Рецензенти

MicheleC зміни затверджено 3 роки тому
SlavekB зміни затверджено 3 роки тому
Запит на злиття був влитиий як 72d4668eea.
Підпишіться щоб приєднатися до обговорення.
Немає рецензентів
Етап відсутній
Немає виконавця
3 учасників
Сповіщення
Дата завершення

Термін виконання не встановлений.

Залежності

No dependencies set.

Reference: TDE/tdegraphics#2
Завантаження…
Тут ще немає жодного змісту.