WIP: Potree library rewriting #1
Assignees
Georg_Hagen
Clear assignees
Georg_Hagen
No Assignees
Oleksii_Hyzha
Oleksii_Hyzha
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Mirrors/PotreeConverter2#1
Reference in New Issue
Block a user
Delete Branch "wip"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
@@ -1,2 +0,0 @@#pragma oncethis has to be the best file in here :D
@@ -140,1 +142,4 @@namespace{std::mutex mut;why is here a mutex in an anonymous namespace in the header?
no one knows. it's not used anywhere. when I was rewriting this lib there were a lot of things that were just declared and not used
@@ -0,0 +4,4 @@#include <filesystem>#include <vector>#include "IPotreeDataProvider.h"#include "converter_utils.h"Always include local headers first and libs & system headers last
This drastically reduces the probability of you creating heads that only work if something else is included before them (as they will already complain about the missing dependencies when you are creating them)
@@ -0,0 +6,4 @@#include "IPotreeDataProvider.h"#include "converter_utils.h"class ConcurrentWriter;not sure what this is forward declaring, but it definitely should be in a namespace
yeah, I was too lazy to put all their stuff in a namespace (though, some of it I actually put xD)
@@ -0,0 +28,4 @@struct NodeLUT{int64_t gridSize;vector<int> grid;Wherever the
using namespace stdis hiding in a header file, please find it and remove itI think it's
using std::vectoron the top of the header. there are a lot of places usingusing@@ -0,0 +11,4 @@bool isDebug = false;static Dbg* instance() {Excuse me, what? :D
Why does this exist and why does it need so many levels of indirection?
I guess it's another debug stuff. I'll remove it
@@ -0,0 +77,4 @@}//inline vector<Attribute> parseExtraAttributes(const LasHeader& header) {//lots of commented out code here
Yes, let's keep it up for now. This function handles extra attributes for las format and I haven't implemented that (most likely we don't need it) because of some complicated (rather hardly readable) code. So, let's leave this block commented out
@@ -4,1 +4,3 @@#include <execution>#include <filesystem>#include <vector>#include "converter_utils.h"see previous comment regarding include order
@@ -5,3 +7,2 @@#include "Vector3.h"#include "LasLoader/LasLoader.h"struct Options;should be in a namespace
@@ -13,3 +13,4 @@#include <array>//#include "LasLoader/LasLoader.h"#include "unsuck/unsuck.hpp"see previous comment regarding include order
@@ -23,6 +24,45 @@ using std::atomic_int64_t;namespace fs = std::filesystem;please get ride of this madness :D
@@ -50,3 +90,4 @@std::vector<ChunkInfo> chunks;};struct State {all these structs should be in a namespace
@@ -335,3 +335,3 @@// taken from: https://stackoverflow.com/questions/2602013/read-whole-ascii-file-into-c-stdstring/2602060inline string readTextFile(string path) {inline string readTextFile(const string& path) {at least they tell us what there source for it is :D
I like the name of file and folder xDD
@@ -473,3 +473,3 @@// it's not very significant, though. ~0.94s instead of 0.96s.template<typename T>inline void writeBinaryFile(string path, vector<T>& data) {inline void writeBinaryFile(const string& path, vector<T>& data) {i don't even want to know :|
@@ -514,3 +514,3 @@// taken from: https://stackoverflow.com/questions/2602013/read-whole-ascii-file-into-c-stdstring/2602060inline string readFile(string path) {inline string readFile(const string& path) {ah, cool, there are 2 read files :D
@@ -531,2 +531,2 @@inline void writeFile(string path, string text) {inline void writeFile(const string& path, const string& text){this file alone is hardcore
with a very telling name too
@@ -0,0 +31,4 @@int currentGridSize = gridSize;int lx = x;int ly = y;int lz = z;why are the copied if the originals aren't used any more?
WIP: First part of cleanupto WIP: Potree library rewriting@@ -13,1 +17,3 @@#set(CMAKE_BUILD_TYPE "Release" CACHE STRING "" FORCE)file(GLOB_RECURSE SOURCES "./Converter/include/*.h" "./Converter/include/*.hpp""./Converter/modules/*.hpp" "./Converter/modules/*.h""./Converter/modules/*.cpp" "./Converter/src/*.cpp")since we are already moving a lot of stuff anyway, can we please also structure this a bit better
can you move the exe related files into a different dir; idk, something like "Examples" and then also move the exe related cmake stuff into it's own cmake file in that dir
and then either move the lass stuff into that exe dir too, or into it's own sub lib
@@ -11,1 +9,4 @@set(EXTERNAL_BROTLI_LIBRARIES "" CACHE STRING "")set(POTREE_ROOT_DIR ${CMAKE_CURRENT_SOURCE_DIR})set(POTREE_LIB_TARGET "PotreeConverterLib")set(LASZIP_DIR "${PROJECT_SOURCE_DIR}/Converter/libs/laszip/laszip")Please break that las related stuff into it's own lib with it's own cmake file
or move it to the exmaple
making it as a lib leads to circular dependencies. laslib needs converter and then you'll link laslib into converter. it's rather an extension for the converter, neither an example nor a standalone lib I'd say
@Georg_Hagen please check ios build error. I guess for ios it needs some extra tweaks with account
It's something about development team, not a toolchain. Though I added development team variable and set some other stuff it still doesn't work.
@@ -56,2 +24,2 @@target_include_directories(PotreeConverter PRIVATE ${EXTERNAL_BROTLI_INCLUDE_DIRS})target_link_libraries(PotreeConverter PRIVATE ${EXTERNAL_BROTLI_LIBRARIES})if(IOS)set(APP_BUNDLE_IDENTIFIER "eu.georgh93.openVulkano")Never reuse a bundle identifier from another project
do I even need it?
every ios software an lib needs a unique bundle identifier
@@ -58,0 +27,4 @@set(MACOSX_BUNDLE_GUI_IDENTIFIER ${APP_BUNDLE_IDENTIFIER})set(MACOSX_BUNDLE_BUNDLE_NAME ${APP_BUNDLE_IDENTIFIER})set(CMAKE_OSX_DEPLOYMENT_TARGET ${DEPLOYMENT_TARGET})set(CMAKE_XCODE_EMBED_FRAMEWORKS ON)Missing
set_target_properties(PotreeConverterExe PROPERTIES XCODE_ATTRIBUTE_DEVELOPMENT_TEAM "466MGSD624")it's set in CMake list of PotreeConverterExe
why are the other here then?
move them together
because these are in root cmake list and define variables for any subproject if needed, while target is defined in it's own cmakelist
and writing
add_subdirectory()
set_target_properties()
instead of configuring target in it's own cmake file is worse
but you only need to configure them in the app target, since you aren't building a shared lib
currently you set target unique values to be the same for the lib, the exe, the json lib, brotli, and laszip
so just move the stuff to the app
app (that is exe) or lib ?
exe the stuff that you can execute
same error
You will need to do a static build of laszip, which you can't because it's lgpl.
Or you will have to properly sign laszip, which isn't worth it.
it complains not only about laszip
error: Signing for "PotreeConverterExe" requires a development team.so, we just disable laszip and data provider impl for ios?
could be that it will not auto create the id from cli
but since your example doesn't work without las and you can't really link laszip
just build the base lib as a static lib for ios
it works without las.
base lib is already built as static
your ci config builds the exe, not the lib, the lib is built implicitly, change the ios config to only build the lib
exe sample can be built without las by disabling POTREE_USE_LASZIP_LIB option
so I can leave exe in config but just disable las
then do that
to test if that properly works need somehow to get rid of that issue
error: Signing for "PotreeConverterExe" requires a development teamalso, please cleanup that las stuff, by using fetch content to get laszip and only download it if enabled
i don't want this LGPL stuff in an MIT project if i can avoid it
@@ -83,3 +70,1 @@list(APPEND LICENSE_PATHS ${BROTLI_DIR}/LICENSE)endif()set(LICENSE_PATHS ${PROJECT_SOURCE_DIR}/Converter/libs/json/LICENSE ${PROJECT_SOURCE_DIR}/LICENSE)we should look for the licenses in the fetched dirs
why do they even copy licenses into executable dir? like licenses are stored in the source dir of certain project why do we need copy them?
because 98% of open source licenses tell you that you need to distribute the license & foss lib name together with the resulting binary
and apps that don't have an ui that can display them, need it somewhere else
then why we don't copy licenses from all libs into dir with exe? =)
Because our app has a 3rd party licenses page on the about page
@@ -10,1 +10,3 @@#include "unsuck/unsuck.hpp"namespace Potree{enum class AttributeType {is this used outside of las?
it is
@@ -156,2 +49,2 @@for (auto& attribute : attributes) {bytes += attribute.size;if (type == AttributeType::INT8) {return "int8";replace with constexpr array
@@ -180,0 +137,4 @@Potree::Vector3 max = { -Infinity };Potree::Vector3 scale = { 1.0, 1.0, 1.0 };Potree::Vector3 offset = { 0.0, 0.0, 0.0 };Potree:: is redundant here
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.