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 ¢er, 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 ¢er, 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.