Page MenuHome

ANT Landscape blender 2.80 Port
ClosedPublic

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

Details

Summary

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

Diff Detail

Event Timeline

Jacques Lucke (JacquesLucke) requested changes to this revision.Dec 27 2018, 10:04 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.

Amir Shehata (amir.shehata) marked 8 inline comments as done.Dec 27 2018, 6:06 PM

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

Amir Shehata (amir.shehata) marked 2 inline comments as done.
Amir Shehata (amir.shehata) edited the summary of this revision. (Show Details)

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?

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.

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?

@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. :)

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.

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.

This revision was automatically updated to reflect the committed changes.