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 force-pushed bug/2765/povray3.7 από το 8bcebad8ec στο 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 force-pushed bug/2765/povray3.7 από το 1f1a0cb713 στο 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 έτη πριν
Το pull request έχει συγχωνευθεί ως 72d4668eea.
Συνδεθείτε για να συμμετάσχετε σε αυτή τη συνομιλία.
Δεν υπάρχουν εξεταστές
Χωρίς Ορόσημο
Χωρίς Αποδέκτη
3 Συμμετέχοντες
Ειδοποιήσεις
Ημερομηνία Παράδοσης

Δεν ορίστηκε ημερομηνία παράδοσης.

Εξαρτήσεις

Δεν έχουν οριστεί εξαρτήσεις.

Αναφορά: TDE/tdegraphics#2
Φόρτωση…
Δεν υπάρχει ακόμα περιεχόμενο.