kpovmodeler not working with povray 3.7 #2

Sloučený
SlavekB sloučil 1 commity z větve bug/2765/povray3.7 do větve master před před 3 roky
deloptes okomentoval před 6 roky
Spolupracovník

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 okomentoval před 6 roky
Vlastník

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 přidal/a PR/not-ok štítek před 6 roky
deloptes okomentoval před 6 roky
Autor
Spolupracovník

AFAIR this option is gone

AFAIR this option is gone
deloptes okomentoval před 6 roky
Autor
Spolupracovník

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 požadované změny před 3 roky
SlavekB zanechal komentář
Vlastník

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.
// if( m_radiosity )
// cl.append( TQString( "+QR" ) );
// else
// cl.append( TQString( "-QR" ) );
SlavekB okomentoval před 3 roky
Vlastník

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 označil tuto konverzaci jako vyřešenou
deloptes vynucené nahrání bug/2765/povray3.7 od 8bcebad8ec do 23b8e49047 před 3 roky
deloptes vyžádal posouzení od SlavekB před 3 roky
MicheleC schválil tyto změny před 3 roky
MicheleC zanechal komentář
Vlastník

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 požadované změny před 3 roky
SlavekB zanechal komentář
Vlastník

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 okomentoval před 3 roky
Vlastník

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 okomentoval před 3 roky
Autor
Spolupracovník

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 okomentoval před 3 roky
Vlastník

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 okomentoval před 3 roky
Autor
Spolupracovník

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 přidal/a PR/wip štítek před 3 roky
deloptes přidal/a 1 commit před 3 roky
deloptes odstranil/a PR/wip PR/not-ok štítky před 3 roky
deloptes vyžádal posouzení od SlavekB před 3 roky
SlavekB posoudil před 3 roky
SlavekB zanechal komentář
Vlastník

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 okomentoval před 3 roky
Vlastník

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 okomentoval před 3 roky
Vlastník

Sounds like a good idea.

Sounds like a good idea.
deloptes přidal/a 1 commit před 3 roky
deloptes přidal/a 1 commit před 3 roky
deloptes okomentoval před 3 roky
Autor
Spolupracovník

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 přidal/a 1 commit před 3 roky
deloptes okomentoval před 3 roky
Autor
Spolupracovník

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 vyžádal posouzení od MicheleC před 3 roky
SlavekB schválil tyto změny před 3 roky
SlavekB zanechal komentář
Vlastník

It looks good. Everything seems to me okay.

It looks good. Everything seems to me okay.
MicheleC požadované změny před 3 roky
MicheleC zanechal komentář
Vlastník

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 okomentoval před 3 roky
Vlastník

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 okomentoval před 3 roky
Vlastník

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 označil tuto konverzaci jako vyřešenou
deloptes vynucené nahrání bug/2765/povray3.7 od 1f1a0cb713 do 72d4668eea před 3 roky
deloptes vyžádal posouzení od MicheleC před 3 roky
deloptes okomentoval před 3 roky
Autor
Spolupracovník

@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 schválil tyto změny před 3 roky
MicheleC zanechal komentář
Vlastník

Looks good now.

Looks good now.
MicheleC okomentoval před 3 roky
Vlastník

@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 schválil tyto změny před 3 roky
SlavekB zanechal komentář
Vlastník

Thanks, now all seems good.

Thanks, now all seems good.
SlavekB sloučil/a commit 72d4668eea do master před 3 roky
SlavekB odstranil/a větev bug/2765/povray3.7 před 3 roky
SlavekB přidal/a toto do milníku R14.0.11 release před 3 roky

Posuzovatelé

MicheleC schválil tyto změny před 3 roky
SlavekB schválil tyto změny před 3 roky
Požadavek na natažení byl sloučen jako 72d4668eea.
Přihlaste se pro zapojení do konverzace.
Žádní posuzovatelé
Bez milníku
Bez zpracovatelů
3 účastníků
Oznámení
Termín dokončení

Žádný termín dokončení.

Závislosti

Nejsou nastaveny žádné závislosti.

Reference: TDE/tdegraphics#2
Načítá se…
Není zde žádný obsah.