-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Planeta again #293
Planeta again #293
Changes from 6 commits
c79dff7
35965e5
680d0aa
20d8c87
41c216f
4ef82be
b4141b7
dd7db8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
/** | ||
* Open Space Program | ||
* Copyright © 2019-2024 Open Space Program Project | ||
* | ||
* MIT License | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in | ||
* all copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
* SOFTWARE. | ||
*/ | ||
#pragma once | ||
|
||
#include <Corrade/Containers/Array.h> | ||
#include <Corrade/Containers/StridedArrayView.h> | ||
|
||
namespace osp | ||
{ | ||
|
||
/** | ||
* @brief Buffer Attribute Format. Describes how to access element attribute data within a buffer. | ||
* | ||
* This is useful for SIMD, GPU, and serialization. A SIMD n-body simulation may prefer [XXXYYYZZZ] | ||
* to store positions, but GPU mesh vertex positions prefer [XYZXYZXYZ...]. | ||
*/ | ||
template <typename T> | ||
struct BufAttribFormat | ||
{ | ||
using View_t = Corrade::Containers::StridedArrayView1D<T>; | ||
using ViewConst_t = Corrade::Containers::StridedArrayView1D<T const>; | ||
using Data_t = Corrade::Containers::ArrayView<unsigned char>; | ||
using DataConst_t = Corrade::Containers::ArrayView<unsigned char const>; | ||
|
||
constexpr View_t view(Data_t data, std::size_t count) const noexcept | ||
{ | ||
return stridedArrayView<T>(data, reinterpret_cast<T*>(&data[offset]), count, stride); | ||
} | ||
|
||
constexpr ViewConst_t view_const(DataConst_t data, std::size_t count) const noexcept | ||
{ | ||
return stridedArrayView<T const>(data, reinterpret_cast<T const*>(&data[offset]), count, stride); | ||
} | ||
|
||
constexpr bool is_not_used() const noexcept { return stride == 0; } | ||
|
||
std::size_t offset{}; | ||
std::ptrdiff_t stride{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. empty-brace-init instead of = 0 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I mentioned this in another review comment: my intention for an empty brace is "don't care what this value is, just not uninitialized." A zero explicitly indicates that the value is important to some business logic somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, it's this one: https://github.com/TheOpenSpaceProgram/osp-magnum/pull/293/files#r1642043900 |
||
}; | ||
|
||
/** | ||
* @brief Builder to more easily create BufAttribFormats | ||
*/ | ||
class BufferFormatBuilder | ||
{ | ||
public: | ||
|
||
/** | ||
* @brief Insert a single contiguous block of attribute data | ||
* | ||
* To make the buffer format [XXXX... YYYYY... ZZZZZ...] for example, use: | ||
* | ||
* @code{.cpp} | ||
* builder.insert_block<float>(count); // X | ||
* builder.insert_block<float>(count); // Y | ||
* builder.insert_block<float>(count); // Z | ||
* @endcode | ||
* | ||
* @param count [in] Number of elements | ||
*/ | ||
template <typename T> | ||
constexpr BufAttribFormat<T> insert_block(std::size_t const count) | ||
{ | ||
auto const prevbytesUsed = m_totalSize; | ||
m_totalSize += sizeof(T) * count; | ||
|
||
return { .offset = prevbytesUsed, .stride = sizeof(T) }; | ||
} | ||
|
||
/** | ||
* @brief Insert interleved attribute data | ||
* | ||
* To make the buffer format [XYZXYZXYZXYZ...] for example, use: | ||
* | ||
* @code{.cpp} | ||
* BufAttribFormat<float> attribX; | ||
* BufAttribFormat<float> attribY; | ||
* BufAttribFormat<float> attribZ; | ||
* builder.insert_interleave(count, attribX, attribY, attribZ); | ||
* @endcode | ||
* | ||
* @param count [in] Number of elements | ||
*/ | ||
template <typename ... T> | ||
constexpr void insert_interleave(std::size_t count, BufAttribFormat<T>& ... rInterleve) | ||
{ | ||
constexpr std::size_t stride = (sizeof(T) + ...); | ||
|
||
(rInterleve.m_stride = ... = stride); | ||
Capital-Asterisk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
interleave_aux(m_totalSize, rInterleve ...); | ||
|
||
m_totalSize += stride * count; | ||
} | ||
|
||
constexpr std::size_t total_size() const noexcept | ||
{ | ||
return m_totalSize; | ||
} | ||
|
||
private: | ||
|
||
template <typename FIRST_T, typename ... T> | ||
constexpr void interleave_aux(std::size_t const pos, BufAttribFormat<FIRST_T>& rInterleveFirst, BufAttribFormat<T>& ... rInterleve) | ||
{ | ||
rInterleveFirst.m_offset = pos; | ||
|
||
if constexpr (sizeof...(T) != 0) | ||
{ | ||
interleave_aux(pos + sizeof(FIRST_T), rInterleve ...); | ||
} | ||
} | ||
|
||
std::size_t m_totalSize{0}; | ||
}; | ||
|
||
} // namespace osp |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,25 +54,14 @@ constexpr std::uint64_t absdelta(std::int64_t lhs, std::int64_t rhs) noexcept | |
/** | ||
* @brief (distance between a and b) > threshold | ||
* | ||
* This function is quick and dirty, max threshold is limited to 1,431,655,765 | ||
*/ | ||
constexpr bool is_distance_near(Vector3l const a, Vector3l const b, std::uint64_t const threshold) noexcept | ||
constexpr bool is_distance_near(Vector3l const a, Vector3l const b, double const threshold) noexcept | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why switch to double? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous implementation here just doesn't work for the Earth-sized distances. I'd rather keep it simple and not write something ultra-optimized here. |
||
{ | ||
std::uint64_t const dx = absdelta(a.x(), b.x()); | ||
std::uint64_t const dy = absdelta(a.y(), b.y()); | ||
std::uint64_t const dz = absdelta(a.z(), b.z()); | ||
double const dx( absdelta(a.x(), b.x()) ); | ||
double const dy( absdelta(a.y(), b.y()) ); | ||
double const dz( absdelta(a.z(), b.z()) ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally i don't find switching from assignment initialization to braced initialization easier to read. Are you doing this to force an error if the type of the result of absdelta is not the same as the type of the variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Not particularly a sane move to do this on my end. Curly braces will enforce the type; however, these are those error-prone round braces that have a silly chance of being interpreted as a function declaration. I'm intentionally implicit-casting int64 from absdelta to a double. |
||
|
||
// 1431655765 = sqrt(2^64)/3 = max distance without risk of overflow | ||
constexpr std::uint64_t dmax = 1431655765ul; | ||
|
||
if (dx > dmax || dy > dmax || dz > dmax) | ||
{ | ||
return false; | ||
} | ||
|
||
std::uint64_t const magnitudeSqr = (dx*dx + dy*dy + dz*dz); | ||
|
||
return magnitudeSqr < threshold*threshold; | ||
return (dx*dx + dy*dy + dz*dz) < threshold*threshold; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could check and see if the codegen is better or worse when using https://en.cppreference.com/w/cpp/numeric/math/hypot here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could def see an argument that using the sum of squares is faster than doing a square root, so i have no concerns about this code. Just pointing out that there's a special function for a different way to slice this. |
||
} | ||
|
||
} // namespace osp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this class is only supposed to be used with ArrayView or StridedArrayView, you might want to consider adding template specializations for only those two, and leaving the "default" template declaration of the struct undefined.
The lazy way of just leaving it like this is also fine, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a requires clause.
either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean a concept for array views?
I'll be lazy on this as there's already a short comment explaining what this type is, and it's a little too simple.