ANT Landscape blender 2.80 Port
ClosedPublic
Actions

Authored by Amir Shehata (amir.shehata) on Dec 27 2018, 2:24 AM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Commits

rBA00fc9bddbcf8: Port 'A.N.T. Landscape' addon to Blender 2.8

Summary

Ported ANT Landscape addon to blender 2.80 as well as its presets

Diff Detail

Repository: rBA Blender Add-ons

Event Timeline

Amir Shehata (amir.shehata) created this revision.Dec 27 2018, 2:24 AM

Amir Shehata (amir.shehata) added a child revision: D4124: Port ant landscapes presets to blender 2.80.Dec 27 2018, 9:17 AM

Thanks for the changes.
Generally looks good to me, except for the small things I pointed out inline.

Also, I have not tried the addon yet, so I don't know if it really works. I expect that you tested it.

ant_landscape/__init__.py
415	Style should be `name: PropertyType()` (no space between the name and colon). Same for all the other property definitions.
566	Looks like this list of enum property items is used in multiple places. I think you should define this list in one place and then use it multiple times to reduce code duplication.
939	Make `classes` a tuple instead of a list (it should not change later on).
940	I'm only reading this on my phone right now and can't see the whole file. The name `panel_func_add_landscape` feels wrong here. Is this actually a class? If yes, then the name should be changed. Otherwise this function cannot be registered this way.
964	Use `for cls in reversed(classes)`. This might not be strictly necessary but might help in cases when there are dependencies between the classes.
ant_landscape/ant_noise.py
567	Why are thos lines not needed anymore? If there is a good reason, they can be removed instead of being commented out.

This revision now requires changes to proceed.Dec 27 2018, 10:04 AM

Ah, just found that you made a second Diff for the presets. I think you could just add the updated presets here.

Made the changes. Will be submitting an update shortly.
And yes, I tested the add-on with its different settings and it appears to be working similar to 2.79

ant_landscape/__init__.py
940	Yes panel_func_add_landscape is a class. Changed it to match the rest of the classes' naming convention.
ant_landscape/ant_noise.py
567	In these lines it appears like previously cellnoise was assigned the value 14. In the 2.8, since we're using "CELLNOISE" directly, we don't need this switching of values. Removed

Addressed JacquesLucke's comments.
Added the update for the landscape presets as part of this diff

Only one little thing (commented inline).

Other than that, seems fine. Will merge later this week.

ant_landscape/__init__.py
964	you accidently added the `reversed` in the register instead of unregister function.

put the reversed traversal in the unregister, instead of the register.

Jacques, I'm probably missing the nuance behind the reverse traversal of the class tuple. Can you describe what potential problem it avoids?

In D4122#93539, @Amir Shehata (amir.shehata) wrote:

put the reversed traversal in the unregister, instead of the register.

Jacques, I'm probably missing the nuance behind the reverse traversal of the class tuple. Can you describe what potential problem it avoids?

In most cases, it has no practical importance/effect. However, in some rare cases, there can be dependencies, and order of register operations is important. In that case, unregistering in reversed order is mandatory too, or some operations may fail (like trying to remove a property from an already unregistered class, etc.). This should actually be the case of the whole unregister func, it should always do everything in reversed order compared to register one, for sake of consistency and sanity. ;)

Makes sense. Thanks for the explanation.

Jacques Lucke (JacquesLucke) accepted this revision.Dec 28 2018, 12:06 AM

This revision is now accepted and ready to land.Dec 28 2018, 12:06 AM

hi, Thanks for the port, appreciated.
Please proceed with commit.
Hopefully the author @Jimmy Hazevoet (jimmyhaze) may find time for some follow up heading towards 2.8 stable.

@Brecht Van Lommel (brecht) I have some problem merging this patch. Not sure what's wrong. For some reason git looks for the wrong paths when applying the diff or so.. Could you merge this maybe?

@Jacques Lucke (JacquesLucke) can’t you just get raw diff (right menu of this page) and apply it?

In D4122#93800, @Bastien Montagne (mont29) wrote:

@Jacques Lucke (JacquesLucke) can’t you just get raw diff (right menu of this page) and apply it?

That is exactly what I tried. Feels like I'm doing some stupid mistake but I'm not sure what exactly..

Ah OK, that’s because this is not a regular git diff, git apply removes by default the first item in paths. You need to do git apply -p0 path/to/D4122.diff and it works like a charm. :)

In D4122#93803, @Bastien Montagne (mont29) wrote:

Ah OK, that’s because this is not a regular git diff, git apply removes by default the first item in paths. You need to do git apply -p0 path/to/D4122.diff and it works like a charm. :)

Ah, that makes sense. I did not know this option. Thank you! Will merge it tomorrow then.

In D4122#93803, @Bastien Montagne (mont29) wrote:

Ah OK, that’s because this is not a regular git diff, git apply removes by default the first item in paths. You need to do git apply -p0 path/to/D4122.diff and it works like a charm. :)

Ah, that makes sense. I did not know this option. Thank you! Will merge it tomorrow then.

Closed by commit rBA00fc9bddbcf8: Port 'A.N.T. Landscape' addon to Blender 2.8 (authored by Amir Shehata <amir.shehata>, committed by Jacques Lucke (JacquesLucke)). · Explain WhyDec 31 2018, 3:04 PM

This revision was automatically updated to reflect the committed changes.