Math node, fixes cleanup and sqrt #17917

Closed
opened 2008-10-27 01:38:03 +01:00 by Aurel W · 15 comments
Member

%%%Hi,

I added sqrt to the math composite node, because I needed it for stuff like this:
http://atommuell.mum.jku.at/~aurel/sobel_filter_suz.png
http://www.blenderstorm.org/qapoll/ideas/idea/683/

I also found a bug in the pow function, it couldn't raise negative values at all. When I tried to fix this i ran into another problem, see BugID 17915. I did a workaround for the math node at the moment tho.

Because the code was rather unreadable too, I cleaned it up a bit. However, I doubt that this 2-4_pixel_processor functions are really a good idea, it might be a little bit more structured code, but it slows down computation massively, compared to just a loop over all pixels. Another issue atm is the labeling in the GUI of the node, it's really not intuitive which input is which variable in operations like x-y, x^y, sin(x),... It might be better to name the inputs X and Y and put names like "sqrt(x)", "x+y" in the drop down box.

Aurel
%%%

%%%Hi, I added sqrt to the math composite node, because I needed it for stuff like this: http://atommuell.mum.jku.at/~aurel/sobel_filter_suz.png http://www.blenderstorm.org/qapoll/ideas/idea/683/ I also found a bug in the pow function, it couldn't raise negative values at all. When I tried to fix this i ran into another problem, see BugID 17915. I did a workaround for the math node at the moment tho. Because the code was rather unreadable too, I cleaned it up a bit. However, I doubt that this 2-4_pixel_processor functions are really a good idea, it might be a little bit more structured code, but it slows down computation massively, compared to just a loop over all pixels. Another issue atm is the labeling in the GUI of the node, it's really not intuitive which input is which variable in operations like x-y, x^y, sin(x),... It might be better to name the inputs X and Y and put names like "sqrt(x)", "x+y" in the drop down box. Aurel %%%
Author
Member

Changed status to: 'Open'

Changed status to: 'Open'
Author
Member

%%%Ok, here's an update, i completely ignored the fact, that the state of an input button is saved to a blend file. In the GUI i added sqrt right to next and changed the other ids, to correspond with the order of the gui. This of course breaks downwards compatibility.

I also changed all the sinf(float),logf,.. functions to the C90 standardized sin(double),... I often here arguments, to stay C90 compliant, which is in most cases a bad idea in the year 2008, but I am not to sure about sinf()

new patch added.%%%

%%%Ok, here's an update, i completely ignored the fact, that the state of an input button is saved to a blend file. In the GUI i added sqrt right to next and changed the other ids, to correspond with the order of the gui. This of course breaks downwards compatibility. I also changed all the sinf(float),logf,.. functions to the C90 standardized sin(double),... I often here arguments, to stay C90 compliant, which is in most cases a bad idea in the year 2008, but I am not to sure about sinf() new patch added.%%%
Member

%%%The cleanup seems pretty good. However, I have a couple of issues.

  1. The pow case shouldn't allow negative numbers to be raised by non-integer powers because that results in imaginary numbers. For example, -4^0.5 = 2i
    You should put in checks for those cases to prevent the pow function from returning an error.

  2. The sqrt case is redundant functionally, because you can accomplish the same thing using pow:
    a^0.5 == sqrt(a)
    There may be a performance advantage in using an explicit sqrt, so I'm not against adding it per-se. But just keep it in mind.%%%

%%%The cleanup seems pretty good. However, I have a couple of issues. 1) The pow case shouldn't allow negative numbers to be raised by non-integer powers because that results in imaginary numbers. For example, -4^0.5 = 2i You should put in checks for those cases to prevent the pow function from returning an error. 2) The sqrt case is redundant functionally, because you can accomplish the same thing using pow: a^0.5 == sqrt(a) There may be a performance advantage in using an explicit sqrt, so I'm not against adding it per-se. But just keep it in mind.%%%
Author
Member

%%%1) Well, that was the fix in the first place. The original code doesn't raise negative numbers at all, now they are raised by integers, or rounded non integers, if closer than 0.001 to an integer (due to this great features of the value button, that needs fixing anyway). Otherwise the result is 0. Maybe you have overlooked that.

  1. sqrt is just used so often, that things get more clear if you explicitly add it I guess. Also 2/3 of the operations in the math node could be removed, if you want to get rid all the redundancies ;)%%%
%%%1) Well, that was the fix in the first place. The original code doesn't raise negative numbers at all, now they are raised by integers, or rounded non integers, if closer than 0.001 to an integer (due to this great features of the value button, that needs fixing anyway). Otherwise the result is 0. Maybe you have overlooked that. 2) sqrt is just used so often, that things get more clear if you explicitly add it I guess. Also 2/3 of the operations in the math node could be removed, if you want to get rid all the redundancies ;)%%%
Member

%%%Ack! You are right. Somehow when I looked at the patch last time I thought all you did was remove the existing (overly conservative) check. I have no idea why, as the code is quite blatant. My apologies.

Regarding the sqrt: I guess the issue for me is just that it feels a little too specific/limited to me. I would feel more comfortable with an n-root operation, for example. But perhaps that's just me. It's a minor quibble in any case.%%%

%%%Ack! You are right. Somehow when I looked at the patch last time I thought all you did was remove the existing (overly conservative) check. I have no idea why, as the code is quite blatant. My apologies. Regarding the sqrt: I guess the issue for me is just that it feels a little too specific/limited to me. I would feel more comfortable with an n-root operation, for example. But perhaps that's just me. It's a minor quibble in any case.%%%
Member

%%%OK... I don't speak english math well... 'raise' is in programming language also used as 'return', so I thought you meant the issue was about the math node not returning negative values at all!

However, the patch still doesnt solve it, it only has a comment about "inaccurate float". Floats are nearly always inaccurate... just proper rounding should solve it. int(y+0.5);
%%%

%%%OK... I don't speak english math well... 'raise' is in programming language also used as 'return', so I thought you meant the issue was about the math node not returning negative values at all! However, the patch still doesnt solve it, it only has a comment about "inaccurate float". Floats are nearly always inaccurate... just proper rounding should solve it. int(y+0.5); %%%
Author
Member

%%%Rounding, that's what the patch does:

if ( x > 0 ) {

  result = pow(x, y);

} else {

  float y_mod_1 = fmod(y,1);
  if ( y_mod_1 > 0.999 || y_mod_1 < 0.001 )
      result = pow(x, round(y));
  else
      result = 0;

}

Really, just an annoying little thing,.... By now this also has to be changed in the copy of the Math Texture Node.%%%

%%%Rounding, that's what the patch does: if ( x > 0 ) { ``` result = pow(x, y); ``` } else { ``` float y_mod_1 = fmod(y,1); if ( y_mod_1 > 0.999 || y_mod_1 < 0.001 ) result = pow(x, round(y)); else result = 0; ``` } Really, just an annoying little thing,.... By now this also has to be changed in the copy of the Math Texture Node.%%%
Member

%%%Hi, trying to update the status of the patch tracker. Please respond if this patch is still viable/useful and needs review or if it can be closed.%%%

%%%Hi, trying to update the status of the patch tracker. Please respond if this patch is still viable/useful and needs review or if it can be closed.%%%

%%%Hi Tom I just starting to study some patches to reach a better idea on how blender code works.
I'll be very grateful if you update the status of this patch. Does it still works?
Thanks
Paolo%%%

%%%Hi Tom I just starting to study some patches to reach a better idea on how blender code works. I'll be very grateful if you update the status of this patch. Does it still works? Thanks Paolo%%%

Added subscriber: @ThomasDinges

Added subscriber: @ThomasDinges

Is this still valid? If not I am gonna close this.

Is this still valid? If not I am gonna close this.
Author
Member

Power is ok, root is not present, code has still strange architecture which is ridiculous unefficient. I think you can close this one.

Power is ok, root is not present, code has still strange architecture which is ridiculous unefficient. I think you can close this one.

Could you elaborate what you mean with "strange architecture"? :)

If there is something we can improve, I am all ears.

Could you elaborate what you mean with "strange architecture"? :) If there is something we can improve, I am all ears.

Changed status from 'Open' to: 'Archived'

Changed status from 'Open' to: 'Archived'
Bastien Montagne self-assigned this 2014-08-10 13:13:13 +02:00

Time to archive.

Time to archive.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#17917
No description provided.