kpovmodeler not working with povray 3.7 #2

Sapludināts
SlavekB sapludināja 1 revīzijas no bug/2765/povray3.7 uz master pirms 3 gadiem
Līdzstrādnieks

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
Īpašnieks

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 pievienoja PR/not-ok etiķeti pirms 6 gadiem
Autors
Līdzstrādnieks

AFAIR this option is gone

AFAIR this option is gone
Autors
Līdzstrādnieks

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 pieprasīja izmaiņas pirms 3 gadiem
SlavekB atstāja komentāru
Īpašnieks

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" ) );
Īpašnieks

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 atzīmēja sarunu kā atrisinātu
deloptes veica piespiedu izmaiņu iesūtīšanu atzarā bug/2765/povray3.7 no revīzijas 8bcebad8ec uz 23b8e49047 pirms 3 gadiem
deloptes pieprasīja recenziju no SlavekB pirms 3 gadiem
MicheleC apstiprināja izmaiņas pirms 3 gadiem
MicheleC atstāja komentāru
Īpašnieks

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 pieprasīja izmaiņas pirms 3 gadiem
SlavekB atstāja komentāru
Īpašnieks

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.
Īpašnieks

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.
Autors
Līdzstrādnieks

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

Sorry, I never knew there is option for radiosity in the GUI.
Īpašnieks

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`.
Autors
Līdzstrādnieks

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 pievienoja PR/wip etiķeti pirms 3 gadiem
deloptes iesūtīja 1 revīziju pirms 3 gadiem
deloptes noņēma PR/wip PR/not-ok etiķetes pirms 3 gadiem
deloptes pieprasīja recenziju no SlavekB pirms 3 gadiem
SlavekB recenzēja pirms 3 gadiem
SlavekB atstāja komentāru
Īpašnieks

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" )
Īpašnieks

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
Īpašnieks

Sounds like a good idea.

Sounds like a good idea.
deloptes iesūtīja 1 revīziju pirms 3 gadiem
deloptes iesūtīja 1 revīziju pirms 3 gadiem
Autors
Līdzstrādnieks

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 iesūtīja 1 revīziju pirms 3 gadiem
Autors
Līdzstrādnieks

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 pieprasīja recenziju no MicheleC pirms 3 gadiem
SlavekB apstiprināja izmaiņas pirms 3 gadiem
SlavekB atstāja komentāru
Īpašnieks

It looks good. Everything seems to me okay.

It looks good. Everything seems to me okay.
MicheleC pieprasīja izmaiņas pirms 3 gadiem
MicheleC atstāja komentāru
Īpašnieks

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 )
Īpašnieks

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.
Īpašnieks

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 atzīmēja sarunu kā atrisinātu
deloptes veica piespiedu izmaiņu iesūtīšanu atzarā bug/2765/povray3.7 no revīzijas 1f1a0cb713 uz 72d4668eea pirms 3 gadiem
deloptes pieprasīja recenziju no MicheleC pirms 3 gadiem
Autors
Līdzstrādnieks

@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 apstiprināja izmaiņas pirms 3 gadiem
MicheleC atstāja komentāru
Īpašnieks

Looks good now.

Looks good now.
Īpašnieks

@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 apstiprināja izmaiņas pirms 3 gadiem
SlavekB atstāja komentāru
Īpašnieks

Thanks, now all seems good.

Thanks, now all seems good.
SlavekB sapludināja revīziju 72d4668eea atzarā master pirms 3 gadiem
SlavekB izdzēsa atzaru bug/2765/povray3.7 pirms 3 gadiem
SlavekB pievienoja atskaites punktu R14.0.11 release pirms 3 gadiem

Recenzenti

MicheleC apstiprināja izmaiņas pirms 3 gadiem
SlavekB apstiprināja izmaiņas pirms 3 gadiem
Izmaiņu pieprasījums tika sapludināts ar revīziju 72d4668eea.
Pierakstieties, lai pievienotos šai sarunai.
Nav recenzentu
Nav atskaites punktu
Nav atbildīgo
3 dalībnieki
Paziņojumi
Izpildes termiņš

Izpildes termiņš nav uzstādīts.

Atkarības

Nav atkarību.

Atsaucas uz: TDE/tdegraphics#2
Notiek ielāde…
Vēl nav satura.