Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The code below is sort of complicated to explain... It writes a "sphere" of data to a 3D array from the given 3D-Position outwards in a spherical shape based on the given radius and value to write.

A graphical representation of this would be:

double UbytePrism :: PrismSphereSet( uint64 Xpos , uint64 Ypos , uint64 Zpos , sint64 Radius , ubyte Byte ) {
    for( sint64 w = -Radius; w <= Radius; w ++ ) {
        for( sint64 h = -Radius; h <= Radius; h ++ ) {
            for( sint64 d = -Radius; d <= Radius; d ++ ) {
                if ( Xpos + w < ByteBuffWidth && Ypos + h < ByteBuffHeight && Zpos + d < ByteBuffDepth && Xpos + w >= 0 && Ypos + h >= 0 && Zpos + d >= 0 ) {
                    if ( ( w * w ) + ( h * h ) + ( d * d ) < Radius * Radius ) {
                        BytePrism[ ( ( ByteBuffWidth * ByteBuffHeight ) * ( Zpos + d ) ) + ( ByteBuffWidth * ( Ypos + h ) ) + ( Xpos + w ) ] = Byte;
                    };
                };
            };
        };
    };

    return 0.0;
};

The general usage of this function would be:

// On a 7x7x7 grid like the image:
// PrismSphereSet( x , y , z , radius , value );
PrismSphereSet( 3 , 3 , 3 , 4 , 5 );

As you can see there is an issue where it's not drawing an exact circular shape. If possible I'd like to fix and or modify this to work elliptical-spheres rather than exact spheres. However, I am not sure how to modify the if-statement if ( ( w * w ) + ( h * h ) + ( d * d ) < Radius * Radius ) { of the function to fit my needs since I pulled it from an example code.

share|improve this question
add comment

2 Answers 2

up vote 3 down vote accepted

There are a lot of simple things you can do to improve this code, but the square is not actually an indication of a problem. You asked for a sphere of radius 4 and your code says that anything that's less than 4 units away should be filled. (I changed this so that things that are exactly 4 units away would be included.) The corners of the square are \$2\sqrt{2} \approx 2.828\$ units away, and therefore are correctly filled. If you manually calculate the distances for each square in the center slice, you'll see they're all "correct" even though it's odd to see a square as the middle slice of a sphere! With larger dimensions, this problem is less noticeable.

Use standard data types

This code uses sint64 as a data type, which suggests a signed 64-bit integer, but there's already a perfectly usable uint64_t which is standard within <cstdint>. Threre are also int64_t and uint8_t types which look like they'd be applicable to this code.

Eliminate spurious semicolons

The code has a number of spurious semicolons immediately after closing braces. They don't bother the compiler but they will bother any other programmer who looks at your code.

Remove loop invariants

Your Radius * Radius code is recomputed every iteration which really isn't needed. Compute it once and use the squared value within the loop.

Use a rational naming convention

This code appears to use CamelCase style names for functions, variables and possibly objects. This hurts readability. Instead, consider using capitalized names for class definitions, an lowercase names for functions and variables. This is a fairly common idiom in C++. It doesn't really matter so much which convention you use; what matters is that you pick one and consistently use it.

Remove pointless return

There seems to be little purpose in the hard-coded 0.0 value returned by the function. Either declare it as void or return something useful.

Use objects

Your code would be considerably simpler using a class to compute distances between the center of your sphere and an arbitrary point in the cube.

Also the code appears to be referring to BytePrism, ByteBuffWidth, ByteBuffHeight, Xpos, Ypos and Zpos without having passed any of them into the function. If that's because they're all class members of a UBytePrism class, that's fine. If they're global variables, then you should change that.

I've reimagined your code with these ideas in mind and it looks like this. First, some includes and then a very minimal Point3d class (with no error checking, so don't use this without addressing that!)

#include <cstdint>
#include <cmath>
#include <cstdlib>
#include <iostream>

class Point3d 
{
public:
    Point3d(int x=0, int y=0, int z=0) : _x(x), _y(y), _z(z) {}
    Point3d &operator-=(const Point3d &p2) {
        _x -= p2._x;
        _y -= p2._y;
        _z -= p2._z;
        return *this;
    }
    int distSquared(int x, int y, int z) const {
        Point3d p(x,y,z);
        p -= *this;
        return p.lengthSquared();
    }
    int lengthSquared() const {
        return _x*_x + _y*_y + _z*_z;
    }
private:
    int _x, _y, _z;
};

Here is a redesigned UBytePrism class that only does cubes. Obviously you can change the shape to any arbitrary rectangular prism if you need to do so:

class UBytePrism
{
public:
    UBytePrism(size_t dim) : _dim(dim), _data(new uint8_t[_dim*_dim*_dim]) {}
    virtual ~UBytePrism() { delete[] _data; }
    int setSphere(const Point3d &center, int radius , uint8_t value );
    uint8_t set(unsigned x, unsigned y, unsigned z, uint8_t val) { 
        return *point(x,y,z) = val;
    }
    uint8_t get(unsigned x, unsigned y, unsigned z) const { 
        return *point(x,y,z);
    }
    friend std::ostream& operator<<(std::ostream &out, const UBytePrism &p) {
        for (unsigned i=0; i < p._dim; ++i) {
            out << "Dimension " << i << ":\n";
            for (unsigned j=0; j < p._dim; ++j) {
                for (unsigned k=0; k < p._dim; ++k) {
                    out << static_cast<unsigned>(p.get(i,j,k)) << ',';
                }
                out << '\n';
            }
            out << '\n';
        }
        return out;
    }
private:
    uint8_t *point(unsigned x, unsigned y, unsigned z) const {
        return &_data[x + _dim * (y + _dim * z)]; 
    }
    size_t _dim;
    uint8_t *_data;
};

This is the member function corresponding to the code you posted:

int UBytePrism::setSphere(const Point3d &center, int radius , uint8_t value )
{
    int r2 = radius * radius;
    int voxelcount = 0;
    for (unsigned i=0; i < _dim; ++i) {
        for (unsigned j=0; j < _dim; ++j) {
            for (unsigned k=0; k < _dim; ++k) {
                if (center.distSquared(i,j,k) <= r2) {
                    ++voxelcount;
                    set(i,j,k,value); 
                } else {
                    set(i,j,k,0);
                }
            }
        }
    }
    return voxelcount;
}

Finally, here's an example of its use:

int main()
{
    UBytePrism prism(21);
    int voxels = prism.setSphere( Point3d( 10, 10 , 10) , 9, 5 );
    std::cout << prism;
    std::cout << "There were " << voxels << " voxels set\n";
}

When run with the parameters given, it reports that 3071 voxels were set. The volume of the sphere of radius 9 is \$ \frac{4}{3}\pi(9^3) \approx 3053\$ so this is acceptably close, considering we're doing all of the math with integers.

share|improve this answer
    
Wow that's a bit much to take in all at once, but amazingly helpful. Normally I'd return something useful, but this is for "GameMaker" which requires a return value. As recommended I'll fix the issues and return the voxel-count as that's an amazing idea. Originally I keep the all CamelCase due to VisualStudio's color-syntax, but if to adopt a better more fluent style I'll go with your suggestion. –  FatalSleep Jul 8 at 16:26
    
As for data types, is it an issue to build my own data types? It seems like it'd take more file size to add in an included file for data types rather than simply defining my own data types. –  FatalSleep Jul 8 at 16:29
1  
Much of the power of C++ comes from defining and using data types that are appropriate to the problem at hand. I generally don't hesitate to define my own data types if I think it will make it easier to understand the code. It's easier to make correct code fast than it is to make fast code correct; don't worry about file size unless it's really an issue. –  Edward Jul 8 at 16:36
add comment

I find it suspicious that your "Dimension 0" is completely empty. I'd at least expect to see a small "+" of 5's there. Whatever shows up in this last dimension should also show up (assuming spherical symmetry), on the top, the bottom and the sides of the sphere. This will round-out your square.

Perhaps you just need to try

if ( ( w * w ) + ( h * h ) + ( d * d ) <= Radius * Radius )

(i.e. <= instead of just <)

But I think you also need to ask yourself - should a sphere of radius 0 have a single 5 at the origin? Should a sphere of radius 1? What exactly does your radius mean?

share|improve this answer
    
I'm glad you didn't have enough rep to comment. This is a productive answer. Welcome to CR! –  ckuhn203 Jul 8 at 11:06
    
Actually the dimension 0 and 6 are correct being empty, it was an issue on my part, I should have put in the OP a radius of 3 which was what I tested with rather than a radius of 4. –  FatalSleep Jul 8 at 16:32
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.